[D6] Gallery link

wrong branch name, 6.x-1.0 should be 6.x-1.x. And remove the 6.x-1.0 tag, it points to an old commit that surely will not be your 1.0 version.

Automated Review

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• ./gallery_link.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    // We add a "data-gallery-id" attribute where we save the gallery id ($counter)
            // If the given preset isn't available we ignore it and use the normal file path
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./gallery_link.info ./gallery_link.js ./gallery_link.module ./README.txt
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Code Reviews
When running the Coder module make sure that it is set to 'minor'. There are also a host of optional add ons that run with Coder and help to spot errors in your code such as Coder Tough Love, the Translation Template Extractor and Text Review to name some of the more popular ones. There is also a really good script called PAReview.sh that does a great job of spotting errors, its still a sandbox project though so is liable to changes.

Manual Review

@file documentation block is missing in the module file, see Doxygen and comment formatting conventions with regards to documenting files

gallery_link.module - line 39
Dont escape apostrophes instead change your outer quotation marks. Using an escape can create trouble for translators.
t('Imagecache preset %preset doesn\'t exist.')
becomes
t("Imagecache preset %preset doesn't exist.")

Other than those points above everything seems kinda clean.

Good luck with the rest of your review.

Hi,

is there a reason you apply for the review process with more than one module? As of http://drupal.org/node/1011698 this is meant to be a one-time process (so one successful application is enough to achieve full project access).

As the review queue is long and there far more applications than active reviewers (which is a real issue IMO and could be solved if more applicants would do reviews theirselves ;)), please decide whether you want rather this project here or #1330914: [D6] Taxonomy term color to be pursued and close the one you do not decide for.

No offense, I hope you understand!

-dave

All your files end with a blank line. This is wrong. Use Drupal core as an example and you will see that no core files have trailing blank lines.

... and yours end with TWO newlines. Again, look at core Drupal to see how it differs from what you've done.

 * @param $items
 * $items is an array which contains the images.
 * $items = array(
 * array('file' => $fid),
 * array('file' => $fid),
 * array('file' => $fid),
 * );

and

if (!empty($item['title'])) {
 $data['images'][$delta]['title'] = $item['title'];
}

Can you update your docblock to explain the optional 'title' key? I don't know where this is coming from, so I can't verify whether it should be run through check_plain() or not (though I suspect it should).

README.txt has used two newlines at the end, which was wrong and is correct now. But now you have removed the single newlines from the other files as well which is not correct.

@klausi: ? What you are saying is obviously not true. Looking at this project as of commit 0da5857b, README.txt currently has a blank line at the end (TWO \n characters), while all the other files have only a single \n (ASCII 0x0A) at the end, as needed and required by the coding standards. This is easy to verify by using hexdump on the files, and I have attached the output below for you to see (make sure you're looking at the right commit). hexdump doesn't interpret the files, it just shows the binary contents, so there is no mistaking what's currently in there. If you see something different, it's because of the editor and/or IDE you're using, which I suspect is Windows-based if it doesn't deal with line endings properly.

Regardless, here is not the right place to discuss problems with PAReview.sh. I suggest we continue this in #1340276: Newline test.

last line of hexdump of gallery_link.info. Note there is exactly one newline at the end:

000000c0  69 65 73 0a                                       |ies.|

last line of hexdump of gallery_link.js. Note there is exactly one newline at the end:

000001f0  74 28 29 3b 0a 20 20 20  7d 29 3b 0a 7d 0a        |t();.   });.}.|

last line of hexdump of gallery_link.module. Note there is exactly one newline at the end:

00001360  70 61 74 68 3b 0a 7d 0a                           |path;.}.|

last line of hexdump of README.txt. Note there are two newlines at the end:

000006e0  22 62 69 67 22 2e 0a 0a                           |"big"...|

The newline issue has been addressed.

yan_nick:
Can you address my comment in #14? Shouldn't take much, just a tweak of the function documentation - and possibly sanitation of the $title variable. (We discussed this in IRC earlier this week, but it doesn't look like it made your code ... though if you feel it's not required, then just explain why.)

Thanks!

Where does 'title' come from? Is it user supplied code which is then output to the screen without sanitization?

If so, than this is a XSS vulnerability, which is a show-stopper from the 'application review' perspective.

Yes, with user provided input it is always best choice to use check_plain. You never know who will be using your module, in which scope and with which permissions granted to whom. (If that is too tough at one place, you will soon enough receive an issue report from someone. Better safe than sorry!) I will have a closer look in this relation in the next round, unless someone else is faster ;)

Your project page shows that you have taken care of it, I like that! Probably it would even improve with one or two screenshots (but does not need to). Also you have already read into the Drupal JS API. Good! :)

Same for doxygen function blocks - except that you omit @return statements, as e.g. in http://drupalcode.org/sandbox/yan_nick/1340136.git/blob/refs/heads/6.x-1... although your function does return something.

I have ran the most recent review script releases on your code; find the results attached.

hth,
dave

doItDave:
Our discussion was whether the module would allow html captions ... so in this case, filter_xss() is probably fine; as check_plain() would eliminate that possibility.

Good catch on the @return statements ... these should be added to the function documentation.

All is clear then right?

Well well, just because the pareview script was wrong on the line endings thing it is not the case that it and drupalcs would not provide important information... didn't you check the latest result? .-(

@jthorson Ok, I was not sure.

Waaah... I should really attach files once I promise to. Argh!

Here you go.

Looks like a false positive to me.

Example from user.module:

watchdog('user', 'New user: %name (%email).', array('%name' => $name, '%email' => $mail), WATCHDOG_NOTICE, l(t('edit'), 'user/'. $account->uid .'/edit'));

Note: No t() call.

See edit above.

Still missing in documentation ... but the reason is that dblog uses t() when displaying the messages ... so wrapping them in t() when saving to the database results in double-translation.

Ooops ... crosspost.

My understanding is that you don't need to use t() in the watchdog() command (which inserts the message into the database), because the t() will be applied when the value is read from the database on output.

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...p709/sites/all/modules/pareview_temp/test_candidate/gallery_link.module
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AND 1 WARNING(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
    57 | WARNING | Line exceeds 80 characters; contains 87 characters
    76 | ERROR   | Arguments with default values must be at the end of the
       |         | argument list
   136 | ERROR   | String concat is not required here; use a single string
       |         | instead
   161 | ERROR   | String concat is not required here; use a single string
       |         | instead
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review:

docblocks

Great that you use docblocks and do it that precise. I like that! Why not use @return in any function that returns a value? ;)

• For line 57, you don't need to mention the param name again as this line refers to it per definition.
• You should have an empty comment line before the @return statement.
• After @return: The function-internal variable is of irrelevant for the docblock. Instead of it, you add the data type(s) of the return value (see http://drupal.org/node/1354#functions for details on all these function declarations).
• Also very good to add links to the api documentation; best practise is to do this in a dedicated @see line. See function docs as well.
• No @author statement in @file blocks. Credits take place in readme.txt.

Concatenation

'/imagecache' will save you one operation per function call. This is no big thing - unless your module runs on a real huge site, where every cpu cycle counts. And it is why the script marks that. ;)

Comment style in line 9

While inline comments should use the // syntax, you want to have /* */ here. And ere another script complains: Start with a capital, end with a period. All comments.

Did we just miss all that before? Have you changed so much recently?

hth, dave

$path = url("file_directory_path()/imagecache/$preset/$path");

Just a matter of precedence, but I'd probably update this to read
$path = url(file_directory_path() . "/imagecache/" . $preset . "/" . $path); ... it becomes more clear to me how the string is being assembled.

Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...view/sites/all/modules/pareview_temp/test_candidate/gallery_link.module
  --------------------------------------------------------------------------------
  FOUND 12 ERROR(S) AFFECTING 12 LINE(S)
  --------------------------------------------------------------------------------
    33 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
    80 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
    91 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
   101 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
   132 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
   134 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
   137 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
   139 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
   141 | ERROR | String concat is not required here; use a single string instead
   144 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
   157 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
   160 | ERROR | Inline comments must end in  full-stops, exclamation marks, or
       |       | question marks
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Source: http://ventral.org/pareview - PAReview.sh online service

Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• "define('FANCYBOX_VERSION', '1.3.4');": all constants that your module defines should start with the module name to avoid name collisions.
• "$path = url("file_directory_path()/imagecache/$preset/$path");": does that work? I don't think you can invoke a function within a string in PHP. Why did you change that?
• @file comment is empty in the module file.
• This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project if you wish.

D6, not D7

This application has had plenty of reviewers look at it, and quick and consistent response from the applicant. There's no reason this should sit for 7 weeks without feedback ... on behalf of the Drupal community, my apologies to yannickoo (and let's push this through!)

Yep, right, let's get this done. (assigning before mlncn commes and takes it :P)

1. Your readme could still have a little more information, have a look at guidelines for in-project documentation.
2. implementation of hook_requirements should be in .install (even in core some of them are not - but they should)
3. Still some comments left that have not a correct ending with . / ! / ?
4. There's some strange handling with the formatter settings, I've already talked with him about this and he's going to fix that in the next minute (Don't forget to remove the unnecessary functions _gallery_link_settings_fancybox_options, gallery_link_settings; the left over configure setting in your .info, and also the call_user_func_array('array_merge', $fancybox_options); should not be needed afterwards)

I'm pretty sure yannick is up to the task and steadily increasing his drupal knowledge to become a great maintainer ;-)

So, thanks for your contribution and welcome to the community of project contributors on drupal.org!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.