[D7] EZ Banner Rotator

Hi muneer1st,

Have you tried to make the JS part with Drupal behaviours? This should help avoiding JS conflicts between different modules scripts. I have attached a file I wrote sometime ago for an old module so you can see how to convert from regular jQuery to Drupal behaviours.

Also, if your module requires a library, I think it should be placed on "sites/all/libraries/libraryfolder" so it can be shared with other modules, and then loaded on your module when needed. If you use libraries folder you can load the script with:

drupal_add_js(libraries_get_path('libraryfolder') . '/script.js');

It also would be great if you use theme functions for HTML output code so other developers can theme it.

Greets!

First off, your master branch still has other files apart than the README.txt.
To remove those:

git checkout master
rm -R ./*
echo See major version branches.> README.txt
git add -A
git commit -am "Removed files from deprecated master branch."

Secondly, PAReview.sh & Coder still show some issues, small ones but still:
http://ventral.org/pareview/httpgitdrupalorgsandboxmuneer1st1200280git

Then, there's an issue with the included javascripts.
Line 290: drupal_add_js(libraries_get_path('ez_banner_rotator') . '/ez_banner_rotator');
Seems to be missing .js from it.

Fix those first, then I'll look at testing it :)

Manual review turned out nothing.
PAReview gives no errors.

As far as I'm concerned, reviewed and tested.

Wait for someone higher up to grant you full project access to git.
Atleast, that's what I'm doing with my module :)

Would you like to take part in the review bonus program? I personally only review/approve applications with a review bonus, but of course you can also wait for other git administrators to take a look at your code.

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

• Drupal Code Sniffer has found some issues with your code (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. Get a review bonus and we will come back to your application sooner.

manual review:

• ez_banner_rotator_help(): "$output = '';": this line is not needed.
• "$data .= theme('pager');": you are not passing any variables to the theme function here?
• admin/structure/ez-banner-blocks/add: Strict warning: Creating default object from empty value in ez_banner_rotator_get_form_add_banner() (line 111 of ez_banner_forms.inc). Please enable strict warnings in your php.ini.
• "$block['subject'] = t($blocks_from_db[$delta]['title']);": As this is user provided data you need to sanitize it before outputting it. Otherwise you get XSS vulnerabilities on your site. Please read http://drupal.org/node/28984
• Reviews of other project applications are missing in your issue summary. It is strongly recommended to do them, see https://drupal.org/node/1011698

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool.

manual review:

1. "update is not a useful commit message. Please see http://drupal.org//node/52287
2. Strict warning: Creating default object from empty value in ez_banner_rotator_get_form_add_banner() (line 110 of ez_banner_forms.inc). Please enable PHP strict warnings in your development environment.
3. ez_banner_rotator_block_view(): this is still vulnerable to XSS exploits. If I enter a block title <script>alert('XSS');</script> I get a nasty javascript popup when the block is viewed. Make sure to read http://drupal.org/node/28984 again.

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 I'll take a look at your project again, right away :-)

Hi,
I did a manual review of your 7.x-1.x branch. Nice work. Here are a few issues I found though:

1. Your master branch is still in the repository and the git clone URL provided points to the master branch. You should either remove your master branch or provide the right git clone URL to your 7.x-1.x branch.
2. All your files in your module seem to have chmod 775. Could you check that?
3. I found a few typos in your README.txt file
   • navitating => navigating
   • direcotry => directory
4. The README.txt file should probably include a "Requirements" sub-section that lists out the jQuery plugin requirements for this module.
5. You need to either add a download link to jquery.backgroundPosition.js or remove the download link to jquery.easing.compatibility.js as you are providing a download link to both these plugins further down in the README.txt file. People who are following the installation procedure step-by-step could get confused.
6. Please remove the tmp/backup file under images/~loading_animation.gif

Hi muneer1st,

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

The git link you provide in your original post is the maintainer version, not the non-maintainer version. Please replace with
git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/muneer1st/1200280.git ez_banner_rotator

Manual Review :
- Usage of t() at line 183, 207, 252, 253 in file ez_banner_rotator.module is incorrect. Use placeholder for passing variables to t().
For example t() at line 207() could also result into XSS if $blocks_from_db[$delta]['title'] contains values like

. Try using t('@title', array('@title' =>$blocks_from_db[$delta]['title'])) .

- Commenting is inappropriate for most of the functions. Instead of writing "Implementation of function foo()". You should give a single line description of what is function is doing.

- Line 217 Describe the variable, what it contains.

- Line 295 - You are using a lot of HTML in return. Try to make your own theme and then apply this HTML through .tpl.php file.

- Line 345 . Try using check_plain($result1->title). You are getting title in $result1 and directly displaying it. Make sure that it does not contain any XSS by sanitizing its data.
- Line 392,393 - Same as 345.

Hi,

some comments:

• You ship an INSTALL file but in this file you refer to the README file that contains the actual information about it. Maybe you can just remove the INSTALL file
• Installation went well.
• Unfortunately, the configuration only permits static width/height numbers. How will this work on smartphones / in responsive design?

Hi,

with your function ez_banner_rotator_generate_banner() its screaming out for a theme template

So implement hook_theme(); link it to a template and put the returned code in the template passing in the var

theme('thebanner',$image_dir);

the same go's for the code in ez_banner_rotator_get_page_banner_blocks_view()

declare $block as an array in ez_banner_rotator_block_info()
declare $items as an array in ez_banner_rotator_menu()

You pick very unique idea and I like your coding style, commenting, uses of core functionality and specially use of TRY-CATCH. I gone through your project issues and it look like you have already addressed most of the issues raised by community.
But I think you have to pay attention on below 2 points.

1. Separate HTML part from module file
know you have already addressed same point raised in #1 by maxtorete but what Drupal suggest is you should put it in separate TPL file. For now you feel that there is no need to modify the HTML structures but we may not estimate the requirement of module user.

2. Readme file having lot of typos
This is not a part of code review but readme file should be typo free as mostly users see only readme file to get your module working

Rest are okay for me.

You pick very unique idea and I like your coding style, commenting, uses of core functionality and specially use of TRY-CATCH. I gone through your project issues and it look like you have already addressed most of the issues raised by community.
But I think you have to pay attention on below 2 points.

1. Separate HTML part from module file
know you have already addressed same point raised in #1 by maxtorete but what Drupal suggest is you should put it in separate TPL file. For now you feel that there is no need to modify the HTML structures but we may not estimate the requirement of module user.

2. Readme file having lot of typos
This is not a part of code review but readme file should be typo free as mostly users see only readme file to get your module working

Rest are okay for me.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

Added Reviewed Project's link