[D7] Google Authorship

HI gabesullice,

Code sniffer shows a lot of non-critical coding standards errors:

http://ventral.org/pareview/httpgitdrupalorgsandboxgabesullice2038225git...

Manual review:

* TODOs should be done before release, especially t().
* Your git branch should be 7.x-1.x. -alpha is a release, which should be a release, not a branch.
* "Implements google_authorship_preprocess_username()" should say "Implements template_preprocess_username()"
* "Implements google_authorship_get_google_id()" should say what the function does. A function does not implement itself.
* "if (strlen($google_id) != 21) {" You should be validating content on input, not output. If someone adds an invalid ID and it doesn't do anything, they won't know why. If you use hook_form_alter() to validate on input, you can tell them why, and even reject invalid values.
* Why is google_authorship_preprocess_username() limited to node views? Why not change username links anywhere they're displayed?
* In google_authorship_enable(), "echo 'True';" looks like a debug statement that might confuse users. If you replace that with feedback intended for users, make sure it gets wrapped in t().

Everything else looks good to me.

Forgot to set status. Looks like some of that was done while I was posting, but I think there's still some left to do.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Some coding style issues with 1.x branch http://ventral.org/pareview/httpgitdrupalorgsandboxgabesullice2038225git...

Manual review:

Line 61, 92,95 google_authorshop.module
und for the language can be improved by LANGUAGE_NONE or a small snippet to detect the language setting.

@gabesullice regarding the language, my suggestion is to use LANGUAGE_NONE instead of und

Sorry if I bring the confusion :)

REF:
https://api.drupal.org/api/drupal/includes%21bootstrap.inc/constant/LANG...

Such as line 71:

From

    // Get the Google+ ID.
    $google_id = $account->field_google_plus_id['und'][0]['safe_value'];

To

// Get the Google+ ID.
$google_id = $account->field_google_plus_id[LANGUAGE_NONE][0]['safe_value'];

I'd suggest getting rid of the README.md file, as it seems likely that will become out of sync with the README.txt file and people wouldn't know which is relevant. But that's not a blocker issue, and I don't see anything major, so marking this RTBC.

Here are some thoughts that are not blockers to approval.

I appreciate that the module does validation on the field which would prevent something like XSS, but it's standard in Drupal to filter on output and the construction of the url has no filtering to prevent problems. Have you tried disabling the validation and seeing what happens if invalid or malicious data is entered for the url? Using the safe_value is a good call in general, but I'm not sure it takes into account the filtering necessary to use the field as a url.

When you get the bundle and nid and do a lot of tests on them, I wonder whether that wouldn't be done better as a menu_get_object? Basically you run the risk that nodes are being displayed somewhere other than node/NID and then this module will stop working. Maybe this module could take action in hook_node_view to populate some data into the $variables for the preprocess function?

@gabesullice - when you've got the review bonus let me know (and if you do any experiments on the above points I'd be interested to hear that too).

Fixed TODOs.

I talked through each of the issues I mentioned in #14 in irc with @gabesullice and he said that the filters are escaped (great) and that he tried the hook_node_view idea and couldn't get it to actually work. Makes sense.

Thanks for your contribution, gabesullice!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

@gabesullice Great job mate!