[D7] Panel Parallaxe

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxslowflyer2426021git

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.

Hi,

Please find my findings. All may not be applicable but ...

You miss the git command to clone the repo.

• panel_parallaxe_init()
• if (strpos(current_path(), '/admin/') === FALSE) {...} can probably be replace by if (!path_is_admin(current_path())) {...}
• drupal_add_css(), CSS_DEFAULT is the default group value so it's not required to add it.
• panel_parallaxe_menu()
• I don't get how it works as the page_argument is "para_admin_form" but the function name in admin.inc is "panel_parallaxe_admin_form".
• panel_parallaxe_admin.inc should probably be panel_parallaxe.admin.inc
• panel_parallaxe_uninstall()
• You should probably use EntityFieldQuery instead of simple db_select().
• js and css files should be named panel_parallaxe.js, panel_parallaxe.panel.css, panel_parallaxe.para.css (or -panel.css & -para.css)
• JS is not following Drupal rules (should use behaviors)

Thanks,

I would suggest to use system_settings_form in admin settings beside that the code looks good and I haven't test the module functionality yet.

Automated Review

[Best practice issues identified by pareview.sh: None found: http://pareview.sh/pareview/httpgitdrupalorgsandboxslowflyer2426021git

Manual Review

Individual user account

Yes Does not follow the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

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.

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. / No: List of security issues identified.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

• One suggestion that’s more of a feature request than a deal breaker (although as a Drupal developer, personally it would be a deal breaker for me). Consider using a new entity type rather than a “Parallaxe” Node. For example, you could use a fieldable panel pane: https://www.drupal.org/project/fieldable_panels_panes
  There are certain implications that go along with using a node that you won’t get with a custom entity or FPP.
• If you use a FPP, you can default things so you won’t have to remember the “build mode” like I kept forgetting.
• I’ve spent a good amount of time in here and see no major issues!

1. (*) Major finding, needs work
2. (+) Release blocker
3. Just a recommendation
4. ...]

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

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.

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

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

  FILE: /home/klausi/pareview_temp/js/panel_parallaxe.js
  --------------------------------------------------------------------------
  FOUND 21 ERRORS AFFECTING 21 LINES
  --------------------------------------------------------------------------
    2 | ERROR | [ ] The second line in the file doc comment must be "@file"
   10 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   11 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   12 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   13 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   14 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   15 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   16 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   19 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   20 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   21 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   22 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   23 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   24 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   25 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   28 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   29 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   30 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   32 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   33 | ERROR | [x] Expected 1 newline after opening brace; 0 found
   34 | ERROR | [x] Expected 1 newline after opening brace; 0 found
  --------------------------------------------------------------------------
  PHPCBF CAN FIX THE 20 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. what are the differences to existing project such as https://www.drupal.org/project/parallax and https://www.drupal.org/project/parallax_bg ? Please add that to your project page.
2. panel_parallaxe.info: why is there no dependency to panels if this is in the Panels package? Please add a comment.
3. panel_parallaxe_preprocess_html(): why do you need the CSS and JS globally on every page? You should only add it if your content is displayed, for example in panel_parallaxe_node_view()?
4. README.txt: why do people have to enable the insecure image_allow_insecure_derivatives setting, why can't you calculate the token?
5. Notice: Undefined index: und in theme_parallaxe_view_theme() (line 86 of panel_parallaxe.module) if there is not image set.
6. This module has a security vulnerability. As part of our git administrator training program i'm assigning this to Naveen so he can take look. If he does not find anything I'm going to post the details about this vulnerability in one week.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

@slowflyer,
Thanks for your contribution.
Manual Review:
+1 as Klausi specified.

1. (+)theme_parallaxe_view_theme : You are passing the data without sanitization.Use the sanitization functions to sanitize the data before passing.Please see
   • https://api.drupal.org/api/drupal/includes!common.inc/group/sanitization/7
   • http://drupal.stackexchange.com/questions/10280/is-check-plain-enough
2. (*)I am getting the warnings as well.Please see the attached screenshot.
3. panel_parallaxe.install : panel_parallaxe_install the variablename $node_example should be more specific like $parallaxe
4. panel_parallaxe_preprocess_html : +@klausi and Also You are adding js/css files using drupal_add_js/drupal_add_css rather use #attached to add the JS/CSS file
   with vars rendered array and where these actually required.
5. panel_parallaxe_menu : For long term I thing you should define the extra permission by your module and use that permission for this page.
6. theme_parallaxe_view_theme : $parallaxe = image_style_url('parallaxe_l_1920_1080', $node->parallaxe[LANGUAGE_NONE][0]['uri']); It will work fine for non-multilingual websites,Use global language object and use the language in place of languge_none.
7. use single quotes where possible for Drupal code standards and a very slight performance benefit.It is used at various places in module like panel_parallaxe_views_api.
8. hook_help() is missing in this module.It would be nice to add it.

I would recommend you, please help to review other project applications to get a review bonus back. This will put you on the high priority list again, then git administrators will take a look at your project right away :)

Manual Review : Read 0789bd5...

1. panel_parallaxe_install :
   

     foreach (_panel_parallaxe_installed_fields() as $field) {
       field_create_field($field);
     }

   Please add a check before that the field already exists using field_info_field so that it will not throw warnings.

2. Please specify on your project page/Readme.txt on the uninstallation of the module it will remove the parallaxe bundle and its nodes.
3. theme_parallaxe_view_theme : You are adding js/css files using drupal_add_js rather use #attached to add the JS/CSS file with $variables
4. theme_parallaxe_view_theme : Please specify on the project page that the first image will be set as parallex image if use will change the configuration later of the image field and make it to upload multiple files.
5. Adding the hook_help is nice

The above are not blockers.So RTBC :)
As all reviewers 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 :-)

Assigning to kscheirer for final review if he has time.

Review of the 7.x-1.x branch (commit 0789bd5):

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

  
  FILE: ...www/d7/sites/all/modules/contrib/pareview_temp/panel_parallaxe.module
  ---------------------------------------------------------------------------
  FOUND 2 ERRORS AFFECTING 2 LINES
  ---------------------------------------------------------------------------
   92 | ERROR | [x] Closing parenthesis of array declaration must be on a
      |       |     new line
   99 | ERROR | [x] Closing parenthesis of array declaration must be on a
      |       |     new line
  ---------------------------------------------------------------------------
  PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ---------------------------------------------------------------------------
  
  Time: 1.17 secs; Memory: 11Mb
  

• 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.

No more objections from last 4 months and no commit after 0789bd5...
None address from #20
No more blocker.

Thanks for your contribution, slowflyer!

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.

oops my bad I forget one step.Please check again it will work for you.