[D7] Unveil

The nice PAReview.sh script. A few things to fix. But, there is a larger problem with the review process for now.

The Project application checklist states

2.3 Ensure your project contains a minimum of handwritten code.
We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed, that means we can't make sure you're able to write secure code and use Drupal's APIs correctly.
If this is the case, we can't approve you as git vetted user (giving you the permission to create full projects). Either we can see that you have the needed knowledge by other contributions within the drupal community or we can promote it to a full project manually.

Right now, the module is at 92 lines. I'm marking this as "needs work" because I'm new to reviewing projects, but it seems like the guidelines want more code in order to be able to grant this permission.

---
Below is from the PAReview.sh script.

Results: http://ventral.org/pareview/httpgitdrupalorgsandboxlorique2019407git

Here are the results for now:

FILE: /var/www/drupal-7-pareview/pareview_temp/unveil.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
10 | WARNING | All variables defined by your module must be prefixed with your
| | module's name to avoid name collisions with others. Expected
| | start with "unveil" but found "preprocess_image_unveil"
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
4 | WARNING | Line exceeds 80 characters; contains 190 characters
7 | WARNING | Line exceeds 80 characters; contains 278 characters
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/unveil.module
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
44 | ERROR | Function comment short description must end with a full stop
50 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
55 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
92 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

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!

I'm not an expert in reviewing, but what I found is that you ask user to copy unveil library to sites/all/liibraries folder (please fix double "i" symbol in your readme file) which is usually used by Libraries API module. So, it would be better to use this module in your case, because it provides a proper way to include external libraries like Unveil.js. So, you could replace this codeblock

if (file_exists('sites/all/libraries/unveil/jquery.unveil.min.js')) {
      drupal_add_js('sites/all/libraries/unveil/jquery.unveil.min.js');
      drupal_add_js('
        (function ($) {
          $(document).ready(function() {
            $("img").unveil(' . variable_get('unveil_preprocess_image_unveiled', 200) . ');
          });
        })(jQuery);',
      'inline');
    }

by something using libraries_get_path('library_name') function. Another thing, are you sure that this piece of javascript code cannot be executed from the file? You should try to implement it in separate file using Drupal behaviors concept (see info about it here). If this is impossible, then you can use inline js code.
It would be better to use drupal_static() function instead of static construction.

Thanks. Good luck!

Thanks for your response.
One thing, if you use drupal_static function you nee to insert & symbol before it (i.e. &drupal_static('unveil_js_added')) to get variable by reference.

t('Unveil js library missing. It should be located at %location%',
          array('%location%' => 'sites/<domain>/libraries/unveil')
        ),

It would be better to remove info about location of the library, because some of users will use 'all' folder instead of domain. Btw, you could implement hook_requirements() and check if library exists here. It would be proper way, I think.

As for js: according to the description of drupal_add_js() function: "This should only be used for JavaScript that cannot be executed from a file." Behaviors allow other modules to override or extend your js code, and this will add some flexibility to your module. It is not big deal, of course. But I'd say that in most cases creating behavior is unnecessary, but nevertheless people use it there.

Thanks.

Hey Lorique,

Just a suggestion but I'd look at adding the t function to your forms to allow multilingual support.

Example,

/**
 * Settings form for unveil.js.
 */
function unveil_settings_form($form, $form_state) {
  $form['unveil_preprocess_image'] = array(
    '#type' => 'checkbox',
    '#title' => t('Enable Unveil.js lazyloading'),
    '#default_value' => variable_get('unveil_preprocess_image', 0),
  );

  $form['unveil_preprocess_image_unveiled'] = array(
    '#type' => 'textfield',
    '#title' => t('Unveiled distance'),
    '#description' => t('The distance from viewport when images are unveiled.'),
    '#default_value' => variable_get('unveil_preprocess_image_unveiled', 200),
    '#required' => TRUE,
  );

Your also storing a lot of variables but I don't see any unveil.install to allow for uninstalling them upon module removal. This would be implemented using hook_uninstall.

As such;

function unveil_uninstall() {
  variable_del('unveil_preprocess_image');
}

Cheers,

Updated status to needs work.

Hi,

I suggest you to remove ventral.org bug :

FILE: /var/www/drupal-7-pareview/pareview_temp/unveil.install
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
2 | ERROR | Missing file doc comment
--------------------------------------------------------------------------------

Some bugs:

file - unveil.module

-- line 109
There is no need to specify 'function' => 'theme_unveil_image', as drupal theme engine will identify this function itself, if neither function or template is given

-- line 153
I installed the module, downloaded unveil library, enabled checkbox in admin/config/development/performance and when i opened a node i found this error message :
Unveil js library missing. It should be located at %location_1% or %location_2%

The variables in t() and its second parameter array should be same, found different variable names - %location_1%, %location_2% and %location%_1 , %location%_2

..

Changing status to needs work

Thanks for implementing proper js.

But I'm really interesting which problems does hook_requirements cause? it seems logical to check requirements in hook_requirements. Your module needs unveil library to be installed, right? So, what is the reason to not check if library missing or not in hook requirements?

Thanks.

I think that your approach is too serious. You shouldn't check version of js library. At least, you don't do this in your current implementation in unveil_add_js() function. So, you shouldn't do this in hook_requirements too. I mean that you can implement something like this:

if (!in_array('unveil', array_keys(libraries_get_libraries()))) {
      $requirements['unveil'] = array(
        'title' => $t('Unveil library'),
        'description' => $t('Unveil library is missing, please view the !readme for installation instructions.',
          array(
            '!readme' => l(t('README.txt'), drupal_get_path('module', 'unveil') . '/README.txt'),
          )),
        'severity' => REQUIREMENT_ERROR,
      );
    }

That's would be enough. And then you could remove this check from unveil_add_js() function.

As for executing of hook_requirements, I think there is some misunderstanding. Hook_requirements is not executed for each page loading (in contrast to hook_init()). You can implement this hook and check it. It fires only during install, update and runtime processes. So, if you check that library is in the directory in hook_requirements once during the installation process, you don't need to check it at every time when the library is needed. In this case you even speed up your code, because you remove the whole if{..}else{...} construction=)

For my opinion, it is not necessary to check some static things like existence of the library. It should be checked only once and then it should be used without any worries. If somebody removes this library from the server... so, it is his fault and you can punish him=)

Thanks.

I enabled and tested your module on a test site, using devel_generate (part of devel) to generate a bunch of content with images, then a view to display them. Here are some notes:

(1) On the admin/reports/status page, when the Unveil library is not installed, the link to the README.txt file only works if you have clean URLs turned on and you are looking at the site in the default language. If not, you might get a link to something like ?q=fr/sites/all/modules/unveil/README.txt, which does not work. Furthermore, with the module installed properly in an environment without clean urls and in a foreign language, I am getting a javascript error in the console: "GET http://localhost/?q=fr/sites/all/modules/1805074/modules/unveil/image_pl... 404 (Not Found)"

(2) The README file says "The unveil js libarary should be located at sites/all/libraries/unveil". I think it is common practice to "reassure" users by giving example path to an actual file, thus adding something like, "so that jquery.unveil.js is at sites/*/libraries/unveil/jquery.unveil.js"

(3) When unveil.js is installed, I'm getting the error Notice: Undefined variable: requirements in unveil_requirements() (line 179 of /path/to/drupal/sites/all/modules/unveil/unveil.module); plus there is no line in the requirements confirming that I installed it correctly. I would suggest not putting your requirement in an if block. If the requirement is met, then reassure the user and say so (I read your qualms about hook_requirements(), which IMHO are not founded -- I have production sites with hundreds of requirements, and everything works fine -- if you still are having problems with hook_requirements, please open an issue for the drupal project, and provide a link to it here).

(4) I'm not sure if this can be remedied, but on a local server the image loading process is fast that one can't readily tell the difference between a site with your module enabled or not. I'm afraid some users might simply think your module is not working at all, so I would add in the README.txt, something like: "To see how this works, open Chrome, then turn on Developer tools (view > developer > developer tools), and use the Network tab to see exactly when your images are being loaded. Scroll along the site and notice that your images are being loaded only when needed".

(5) The README.txt is a bit too succint. I would suggest that the part about disabling unveil.js (a) be in its paragraph, and not in the same sentence as how to enable it; (b) an example be given of how to add "exclude_unveil to the image settings using preprocess" (otherwise you'll have issues filed asking exactly that question).

(6) I wouldn't mind having a bit more documentation on js/unveil.behavior.js.

(7) Finally, I'm curious as to why you don't enable this by default when the module is on. Why do we need to set it in the performance page? If someone enables this module on a site, I would prefer it to "just work".

Probably the only show-stoppers here are clean urls and foreign language prefix support, so I'm setting this to Needs Work. Other than that, good work!

Albert.

This looks good.

I'm not quite sure why there is a checkbox for unveil_preprocess_image on both the performance page (via form_alter) and the unveil settings tab (via unveil_settings_form). Also, they have different default values, 0 in the first case, 1 in the second case - they should at least be consistent. There are also a few issues to take care of listed here: http://pareview.sh/pareview/httpgitdrupalorgsandboxlorique2019407git.

That's not a blocking issue though, the module looks good!

Thanks for your contribution, lorique!

I updated your account to let you 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 get 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.

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