[D7] Fullscreen gallery

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.

Manually code review (without installation)

fullscreen_gallery.module

• 9, 10 - double empty line, are you serious?
• 14 - there is no necessity of comment for hook_menu();
• 19 - looks like you are missing 'title' key;
• 70, 70 - I think that the title and description of the permission should begin with a capital letter.

fullscreen_gallery.css

• 146, 149 - extra space between 'a' and pseudo-class 'hover'.

fullscreen_gallery.js

• 177, 178 - doubled jQuery selector;
• 375, 377 - grammar typo in 'resizeing' and 'reinitalize'.

_fullscreen_gallery_page:168:

      // Set entity title.
      $title = isset($entity->name) ? $entity->name : '';
      $title = isset($entity->title) ? $entity->title : $title;

I think it would be clearer to write this out as a if-else block.

_fullscreen_gallery_page():174:

      $content['fullscreen_gallery_right'] = render($blocks);

This is really a feature request, but I would suggest letting the user decide on the regions and blocks. Take a look at dashboard module. I know this is a significant feature request, but at a minimum, I would suggest to include an option for left sidebar as well.

The default fullscreen_gallery.tpl.php has a lot of div's. See if you can make them more semantic. I am thinking you can use figure and figcaption tags.

I don't think any of these are significant, just small improvements and hence not setting to Needs work.

Hi zbombicz,
Thank you for providing these module,
i cross checked copy of these module with coder module and i got some very minor issues can you please look into it.

3 screen shot has been attached.

Thanks,
Rahul.

Your hook_help() is designed to use Markdown filter (if available) to render the content, but text is not properly "marked up" for markdown. This patch remedies that.

Automated Review

PAReview came up clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Does not inform about module duplication and/or fragmentation.

There is already exists a lot of gallery modules for Drupal. Here is a list with those I am aware of:

• bootstrap_gallery
• gallery
• galleryformatter
• juicebox
• photos
• plus_gallery
• swipe_photo_gallery

The way I interpret § 1.2 of the Project application checklist, is that we only block if the "differences between your modules are not too fundamental for patching an existing one".

However, users need to be informed about possible functional overlap. This should be made is section with the heading "Similar projects and how they are different" on the project's project page.

1. acknowledges the existence of similar projects; and
2. briefly explain how they are different.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

Project page

No. Please take a moment to make your project page follow the Project page template and it may be a good idea to also read tips for a great project page.
In particular, you should add the section "Similar projects and how they are different".

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

No blocking issues found.

The only blocking issue I've found is that the project page needs a section about similar projects and how they are different.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Thanks for committing my patch. Please note that the best practice when committing a patch authored by someone else is to credit authorship with the --author option to git. You'll find a suggestion for the command to use if you scroll down to the bottom of this issue tracker and expand the "Credit & committing" tab and look under the "Git command" tab. In this case, the Git command should have been:

git commit -m 'Issue #2465165 by gisle: [D7] Fullscreen gallery' --author="gisleh <gisleh@409554.no-reply.drupal.org>"

You don't have to do anything now. This is just FYI to tell you about best practices for responsible maintainership.

Your project page now has a section about similar projects and how they are different.

There are IMHO no more blockers. Moving to RTBC. Note that promotion will not happen until a git administrator has given this a second set of eyeballs.

Nice project!
Review of the 7.x-1.x branch (commit 6095f13):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

Manual Review :

1. fullscreen_gallery_admin : Call the variables with their default values in variable_get function.
2. fullscreen_gallery_help : We don't need to sanitize the text of the read me file because it is trusted and provided by the module.
3. fullscreen_gallery.tpl.php,page_fullscreen_gallery.tpl.php : It would be nice to document the available variables in the template
4. (+)_fullscreen_gallery_page : $content['image_title'] = fullscreen_gallery_get_title($photos, $img_num, $title, $display_settings); Here is the need to sanitize the data that is passing in image_title because the data is directly passsing in fullscreen_gallery.tpl.php and it is also not sanitized there.So either add sanitization function here/Add it in the template file.
5. fullscreen_gallery.css : The file is directly applying css on the basic css elements clear and anchor it might break the default css provided by the theme. can't we use the default clearfix class ? Please add a comment.

Removing the review bonus.Please take another review bonus for 2nd admin review.

Please don't RTBC your own issues, see the workflow: https://www.drupal.org/node/532400.
Review of the 7.x-1.x branch (commit 70b6f52):

1. _fullscreen_gallery_page : Its prefferable to use #attached in the content render array over drupal_add_js,drupal_add_css.
2. Currently module is adding a specific region 'fullscreen_gallery_right'.There should be feature that user should select the region.

Rest seems good to me.The status is same RTBC.
You need to get another review bonus by manual reviews of another 3 modules that would help speed up the process and will list you in the review admins radar.

Review of the 7.x-1.x branch (commit 70b6f52):

1. #19.1 : $content is a render array so you can use #attached with $content array before calling theme function.

Assigning to @klausi to give it a final look if he has time.

Review of the 7.x-1.x branch (commit 70b6f52):

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/js/fullscreen_gallery.js
  ----------------------------------------------------------------------
  FOUND 4 ERRORS AFFECTING 4 LINES
  ----------------------------------------------------------------------
    51 | ERROR | [x] Expected newline after closing brace
   190 | ERROR | [x] Expected newline after closing brace
   227 | ERROR | [x] Expected newline after closing brace
   337 | ERROR | [x] Expected newline after closing brace
  ----------------------------------------------------------------------
  PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ----------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

manual review:

1. _fullscreen_gallery_page(): do not call theme() and render() here, just return a render array that can be easily altered by other modules. See https://www.drupal.org/node/930760 . You should also use #attached for your JS/CSS on the render array, see https://api.drupal.org/api/drupal/developer--topics--forms_api_reference...
2. _fullscreen_gallery_page(): this is vulnerable to access bypass exploits. If I have an image field protected with restricted permissions then an unprivileged user can still view the image via the /fullscreen_gallery/... path. Example: If I use field_permissions module to set the image field to private an a node article, then anonymous user will not see the image on the standard node view page (correct) but they can view it in the fullscreen gallery (access bypass). You need to check with field_access() if the current user is allowed to see the image. I think you should add the field parameters to your fullscreen_gallery_access_callback() and call the field_access() there. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
3. You could use "%entity_object" in your hook_menu(), so that the menu system does the entity loading for you. See entity_object_load() docs.
4. fullscreen_gallery_set_default_theme_variables(): why is that necessary, you can set the default values for all the variables in hook_theme()? See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...

The Git commits are not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 and https://www.drupal.org/node/1051722

Since this was already RTBC already and the field access security issue was the only hard blocker ...

Thanks for your contribution, zbombicz!

I updated your account so you can 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 stay 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.