Plus Gallery

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and put yourself on the PAReview: review bonus high priority list. Then I'll take a look at your project right away :-)

Hello,

Automated code review shows nothing :)
http://ventral.org/pareview/httpgitdrupalorgsandboxseppemagiels1942084git

From manual review:
Generally code looks very perfect!

Just found couple things:
1. plus_gallery.module@530 | $lastpart unused variable
2. You ask to "Go to http://plusgallery.net and download the latest version. Then uncompress
the contents of the "plusgallery" directory of the downloaded file to this
folder (modules/plus_gallery/plusgallery)."
I suggest to use sites/all/library folder for external libraries.

Nice module. Good luck!

As already mentioned, the automatic review shows no errors.

In my manual review, I also didn't see anything problematic. The code looks really clean and well-written. The only thing I might mention is a really minor inconsistency. You separate your markup & calls to t() and concatenate them, which is very helpful to translators. In some places, though, you have a colon outside the t() call (plus_gallery.module, lines 120-121) and in others, you have them inside (plus_gallery.module, lines 73-79). Maybe that was intentional but I figured I'd point it out. Not a major deal at all, though.

Hey, those nits aren't gonna pick themselves, right? It's what I do. :-)

--llang

Overall, good job. Code is generally well written and easy to read. Few things to take care of:

** */*

The code uses both single and double quotes, it would be better to use just one type of quotes.

** plus_gallery.module, lines 33, 490

I am confused by the usage of this variable. Shouldn't you use the Libraries module to load 3rd party code?

** plus_gallery.module, line 38

It would be nice to add a description to this permission.

** plus_gallery.module, lines 236, 239

Some of the lines are very long. There are few exceptions to this rule, but in general lines should be shorter than 80 characters.

** plus_gallery.module, line 377

Please double check that you did not forget to put a break keyword in this clause. If break is not needed here, you can put a Falling through comment to show that this is the intended behavior.

** plus_gallery.module, line 547

Do not manually create a list of attributes. Use drupal_attributes().

** plus_gallery.module, line 549

You should not use jQuery().ready(...) in Drupal. JavaScript should be initialized with Drupal behaviors.

** plus_gallery.module, line 555

One does not simply replace jQuery in Drupal. If +Gallery does not work with jQuery v1.4.4 (which is used in Drupal by default), the correct way to update it is to use the jQuery Update module. You should add it to the list of dependencies.

** plus_gallery.module, lines 580, 588, 596, etc.

Where does the variable $title come from in this code?

'message' => t('The maximum length of the %name is %length characters.', array('%name' => t($title), '%length' => $length)),
    

Can the value of $title be changed by the site administrator or simple user? In that case, you should remove the t() call from this line, because user-defined strings should not be translated with t().

In any case, this usage of t() is wrong. You'd want to read its documentation for details.

** plus_gallery.install

Some of the lines contain tabs. You should never mix tabs and spaces for indentation.

** plus_gallery.admin.inc, lines 28, 36

Documentation of the plus_gallery_save_settings() function is inaccurate. It says that it is a validate handler when it is, in fact, a submit handler.

** plus_gallery.admin.inc, line 32

Usage of system_settings_form() function is not entirely correct. It has its own submit hander. The Core has a bunch of examples of the correct usage.

I've reviewed the code for this module. It adheres to Drupal's coding and commenting standards, and is clean and well-structured. I particularly enjoyed the in-depth integration with the Field system and the thorough error handling. Also, I think the integration with outside photo services will make it very popular. I only located a few minor problems:

README.TXT
You instruct the user to download +Gallery to sites/all/library, when I think sites/all/libraries is the correct name for integration with Libraries API.

plus_gallery.install
hook_requirements() implementation: Items in your $requirements array should have a key set, as in requirements['key'] = array(...

plus_gallery_update_7100()
You're concatenating the return value of _field_sql_storage_tablename($field) and _field_sql_storage_revision_tablename($field) with $field['field_name'] on line 169. I think the return values of these functions already includes the field's name.

All hook_update_N() functions should take the parameter &$sandbox, though I don't think you'll need to actually use it.

plus_gallery.module
Line 737: $options[PLUS_GALLERY_GOOGLEPLUS] = t('Goolge+');
'Google+' is misspelled

plus_gallery_field_formatter_view() line 566:
I see that for each delta of the field you're adding inline JavaScript that defines the attach function for Drupal.behaviors.plusGallery. Won't each iteration overwrite the previous, meaning only the last delta will actually work? Even if not, I think a better approach would be to add $(".plusgallery-' . $delta . '", context).plusGallery(); for each delta to a variable, then have one call to drupal_add_js after the foreach. I'm thinking something like:

$selectors = '';
foreach ($element as $delta => $item) {
  selectors .= '$(".plusgallery-' . $delta . '", context).plusGallery();';
}
drupal_add_js('
  (function($) {
    Drupal.behaviors.plusGallery = {
      attach: function(context, settings) {
        ' . $selectors . '
      }
  };
})(jQuery);', 'inline');

manual review:

1. Your module file is quite long. Have you considered moving stuff to include files, for example plus_gallery.field.inc?
2. plus_gallery_permission(): the permission is never used?

But otherwise this looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to jthorson as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, Seppe Magiels!

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.