[D7] Scroller Block

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

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.

I have installed , configured and review your module code, every thing is perfect.

Thanks

Hi DHL,

1. When enabling the module I got the below errors. The directory name is obviously wrong on my part but it shows a problem with your include. It seems odd that you're doing this from here anyway. 'sites/all/modules/scroller_block/scroller_block.module' shouldn't be hard coded. You should be using "drupal_get_path('module', 'scroller_block')". The include should be within your hook_install() etc. If you need to leave it where it is, simply use "include_once scroller_block.module;"
   

   Warning: include_once(C:\wamp\www\drupal/sites/all/modules/scroller_block/scroller_block.module): failed to open stream: No such file or directory in include_once() (line 8 of C:\wamp\www\drupal\sites\all\modules\2135613\scroller_block.install).
   Warning: include_once(): Failed opening 'C:\wamp\www\drupal/sites/all/modules/scroller_block/scroller_block.module' for inclusion (include_path='.;C:\php\pear') in include_once() (line 8 of C:\wamp\www\drupal\sites\all\modules\2135613\scroller_block.install).

2. You're also using a lot of Drupal variables, resulting in unnecessary database calls. You should have all those settings in an array and add them to one "settings" variable.
   

    variable_set('scroller_block_bounceeffect', $edit['scroller_block_bounceeffect']);
       variable_set('scroller_block_bounceheight1', $edit['scroller_block_bounceheight1']);
       variable_set('scroller_block_bounceheight2', $edit['scroller_block_bounceheight2']);
       variable_set('scroller_block_bouncespeed1', $edit['scroller_block_bouncespeed1']);
       variable_set('scroller_block_bouncespeed2', $edit['scroller_block_bouncespeed2']);
       variable_set('scroller_block_velocity', $edit['scroller_block_velocity']);
       variable_set('scroller_block_direction', $edit['scroller_block_direction']);
       variable_set('scroller_block_startfrom', $edit['scroller_block_startfrom']);
       variable_set('scroller_block_loop', $edit['scroller_block_loop']);
       variable_set('scroller_block_movetype', $edit['scroller_block_movetype']);
       variable_set('scroller_block_mouseover', $edit['scroller_block_mouseover']);
       variable_set('scroller_block_mouseout', $edit['scroller_block_mouseout']);
       variable_set('scroller_block_startup', $edit['scroller_block_startup']);
       variable_set('scroller_block_cursor', $edit['scroller_block_cursor']);
   

3. Your array declarations seem unnecessarily verbose:

   $scroll_startfrom = array();
   $scroll_startfrom['right'] = t('Right');
   $scroll_startfrom['left'] = t('Left');
   $scroll_startfrom['top'] = t('Top');
   $scroll_startfrom['bottom'] = t('Bottom');
   

   Would be better as:

   $scroll_startfrom = array(
     'right' => t('Right'),
     'left' =>  t('Left'),
     'top' => t('Top'),
     'bottom' => t('Bottom'),
   );
   

4. Your title on this issue is a little off, it should start with "[D7]" rather than "[D7}" ;)

1. You can help other developers find the module configuration by adding a 'configure' line in your .info file, check out this page for more information: https://drupal.org/node/542202
2. You should add 'block' as dependency in your .info file

• You have a typo in the readme, "Mrquee".
• $scroll_mouseover, $scroll_mouseout, $scroll_startup are the same set of options, you could just use 1 for all 3 selects.
• The Libraries API module is a recommended method for adding 3rd party dependencies. This allows the admin to use more paths for required libraries. This is not a required change though, just a recommendation. The same with a hook_requirements() to check that the library was installed properly.

Looks like a nice module, no major issues found.

----
Top Shelf Modules - Crafted, Curated, Contributed.

You can leave it in RTBC, those were not blocking issues. There's still one level of review left.

Automated Review

1.  scroller_block.module
   Variable 'output' might have not been defined (at line 376)
2. scroller_block.css
   Font property font-family does not have generic default at line 15
3. scroller_block.css
   Unit of measure px is redundant at line 37 & 38

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No.

scroller_block.module
$output .= '<span>' . $message . '</span>'; on line 338.
This isn't run through check_markup(). Provides the opportunity for XSS and other nasty stuff.

Coding style & Drupal API usage

1. (+) scroller_block.install
   Links aren't built using l() on lines 44 & 59.
2. (+) scroller_block.install
   

   if (file_prepare_directory($dir)) {
       file_unmanaged_delete_recursive($dir);
     }
     db_delete('file_managed')
       ->condition('uri', '%scroller_block%', 'LIKE')
       ->execute();

   Use file_delete() rather than unmanage delete. This will call all the appropriate file delete hooks.

3. scroller_block.page.inc
   Consider using t() for the link to wikipedia (and not l()) as there are language specific urls that could be used.

         $output = '<p>' . t('Reference to: !wikilink | !maxvergelli.', array(
           '!wikilink' => l(t('Wikipedia'), 'http://en.wikipedia.org/wiki/Scrolling'),

4. (+) scroller_block.js
   

   Drupal.behaviors.scroller_block = {
        attach: function(context, settings) {
          scrollerBlock();
        }
     };

   This doesn't pass (and use) context & settings into scrollerBlock(). It should. There's no need to grab $loop from the global Drupal namespace on line 8 if you pass in settings. And use context on line 14.

5. (+) scroller_block.module
   

   function scroller_block_help($path, $arg) {
     module_load_include('inc', 'scroller_block', 'includes/scroller_block.page');
     return module_invoke('scroller_block', 'help_delegate', $path, $arg);
   }

   module_invoke is slow, just call the function.

6. (+) scroller_block.module
   

     drupal_add_css($path . '/css/scroller_block.css');
     drupal_add_js($path . '/js/scroller_block.js');
   }

   Lines 301, 302 and 331, use #attached rather than drupal_add_*

The starred items (*) are fairly big issues and warrant going back to Needs Work. The items with a plus (+) are important issues and strongly encouraged to fix. The rest of the comments in the code walkthrough are recommendations.

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

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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