[D7] Responsive Lazy Loader

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

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 jetmartin,

Looks like an interesting module. I see you almost pass PAReview! Some pointers:

• You should add the README.txt file to your module, detailing how to use and install (dependencies, etc.). Don't wait until your application is approved, as it gives us all more info on your module.
• When configuring your module on a fresh D7 installation, it says $conf['image_allow_insecure_derivatives'] = TRUE; must be added to settings.php. You should add that to the installation instructions as well.
• On responsivelazyloader_process_html() it basically adds an inline javascript to $vars['page_bottom'], but that way Drupal can't take advantage of js aggregation. Perhaps you can split your code in two to fix this:
  1. A settings part, and add it to Drupal's settings javascript object:
     drupal_add_js(array('responsivelazyloader' => $your_settings_array), 'setting');
  2. A regular javascript file, where you retrieve your module's settings by using Drupal.settings.responsivelazyloader.

  That should allow aggregation while using variable settings.

I'll keep testing your module and giving you feedback.

Your post also lists your personal git URL, change it to your public git clone command:

http://git.drupal.org/sandbox/jetmartin/2158105.git

Hi

Please rename readme.md to README.md. It's even showing up as en error in pareview.sh.
Verify there are no comments/errors on pareview before changing the status further.

Thanks
Raman

Hi

Under the file responsivelazyloader.init.js.

    /* Lazy Loader init. */
    $("img[data-src]").responsivelazyloader();
    /* Lazyloader init after AJAX call if jQuery>=1.7 */
    var arr = $.fn.jquery.split('.');
    if(arr[0] > 1 || (arr[0] == 1 && arr[1] > 7)){
      $("body").on({ ajaxStop : function(){ $("img[data-src]").responsivelazyloader(); }});
    }

1. The comment and code mismatches, the comment is for jQuery >= 1.7 while the if condition only validates for jQuery > 1.7.
2. Also, I tested with jQuery 1.8 and the ajaxStop doesn't fire. Please add it to document instead of body, see the additional notes section on this page. You can also refer to the answer here.
3. Also, for jQuery > 1.7, wouldn't the call be made twice? Once above the if statement and one inside the if statement? Or am I missing something?

Thanks
Raman

You should name folder with module responsivelazyloader as your responsivelazyloader.module file.

You should rename README.md to README.txt. It's a standart for Drupal.

responsivelazyloader.info:
Some typos:
description = Enable a full configurable responsive lazyloader. You cand create custon configuration for any CSS grid system.

stylesheets[all][] = css/responsivelazyloader.css
scripts[] = js/responsivelazyloader.init.js

These files is added to all pages including page where they are not using. You shoud add it via drupal_add_js() and drupal_add_css() in .module file.

responsivelazyloader.module

define('RESPONSIVELAZYLOADER_MODULE_PATH', base_path() . drupal_get_path('module', 'responsivelazyloader'));

You are using it only once. Are you sure it should be a constant?

Hook_init(). should be Implements hook_init().. Also everywhere.

  // Load the jQuery plugin if available.
  if (module_exists('libraries')) {
    $path = libraries_get_path('responsivelazyloader');
    if (file_exists($path . '/jquery.responsivelazyloader.js')) {
      drupal_add_js($path . '/jquery.responsivelazyloader.js');
    }
    if (file_exists($path . '/jquery.responsivelazyloader.min.js')) {
      drupal_add_js($path . '/jquery.responsivelazyloader.min.js');
    }
  }

These files also is added to all pages.

'#markup' => '<noscript><style type="text/css" media="all">img[data-src] { display: none; }</style></noscript>',
See the proper version of adding noscript tag https://drupal.org/comment/6840288#comment-6840288

responsivelazyloader.admin.inc

/**
 * @file
 * ResponsiveLazyLoader Admin.
 */

It should be like this:

/**
 * @file
 * Code for admin pages for ResponsiveLazyLoader module.
 */

"JQuery responsivelazyloader must be installed" should be "jQuery responsivelazyloader must be installed".

Hey - this module looks great. Can't wait for a release.

I'll check out the module in the meantime and provide any feedback that comes to mind.

Thanks :)

Luke

Hi JetMartin,

Wow a great module you have here I am looking forward to seeing this in the wild.

I checked and there are no issues from http://pareview.sh, however there are some small things I picked.

• It's a small thing but on line 107 & 113 you have a line break in the t( I think for consistency it would be better not to break lines here.
• Extra space on line 16 of .install
• Again a small thing I know but there is quite a few punctuation and spelling issues in the read me file. I can list if you are struggling but there was quite a few I could see.

They are small things and can be quite frustrating but it looks like you are quite close. I need to go through this again in a bit more detail later to see if I can spot anything that might be an issue.

Great work!

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

Hey ... any more news on this app?

Hope your vacation was good.

L

I recently created a patch for this module in which I reviewed the spelling issues (https://www.drupal.org/node/2298185).

If BigEd or someone else could review this module again and eventually make a list of the remaining spelling and punctuation issues, that would be great!

Hey,



Following is the review -

Automated Review

pareview.sh still identifies a few errors and warnings. Please go through them here - http://pareview.sh/pareview/httpgitdrupalorgsandboxjetmartin2158105git

Manual Review

No duplication

No: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

README.txt/README.md

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

Coding style & Drupal API usage

1. Permissions are not really required for this functionality. It should be only available to the admin to set or not once and for all for the entire site.

Good module, serves the purpose well. Though for future development(after release) you could make application of this functionality configurable like field/image formatters.

This review uses the Project Application Review Template.

Manual Review

README.txt/README.md

No: does not follow the README Template but README file is present.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Security

No. image_allow_insecure_derivatives is a requirement of the module, why? Is there a way we can go around this by generating image style derivatives on the server side and using Drupal javascript settings to inject the paths? This is a security issue, and opens any site using this module up to a DDOS style attack. I think this alone would be reason enough for people not to install this module on their site as providing a lazy loader at the expense of opening a site up for attack doesn't seem worth it. If you would like help with making this module not have to bypass derivative security then I can help, just let me know why it's required at the moment (what piece of code is being broken).

Coding style & Drupal API usage

List of identified issues in no particular order:

1. (+) The security issue mentioned above
2. (+) responsivelazyloader.init.js do not use document.ready, use Drupal behaviors. In the current state the module breaks with any AJAX loaded content but using Drupal behaviors this will no longer be an issue. Please review the Behaviors section of https://www.drupal.org/node/756722 - its basically like doing a document.ready however you're doing it on every request and sub-request and you have access to changing Drupal.settings when AJAX commands make modifications.
3. (+) Indenting for arrays / array values is different from how Drupal core and contrib modules are usually written. For example in hook_menu the items array, the array values are all lined up. Though this is accepted in some other frameworks this is not commonplace in Drupal and makes it difficult for other maintainers or patches to contribute.
4. (*) Weight of module is excessive and prevents any modules from overriding it. If there is a specific module causing problems then mention it here, otherwise reduce the weight to something more sensible like 100. You can see a common list of module weights at https://www.drupal.org/node/110238. One of the uniquely awesome things about Drupal is that modules can override each other.

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.

Great work otherwise, I can really see potential for this module. My only real concern is the security issue and I think we can fix that.

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.

Hi, any update on this? I'd be happy to help out if I can.