[D7] Jquery Touch Swipe

Hi, rafuel92

Please review: http://pareview.sh/pareview/httpgitdrupalorgsandboxrafuel922045643git

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.

It appears you are working in the "7.x" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch.
For additional resources please see the documentation about release naming conventions and creating a branch in git

You should work on a "7.x-1.x" branch.

And also you should specify in your README.txt file and in your description that your module depends on libraries module, and put a direct link to libraries module.

Code too short

This project is too short to approve you as git vetted user. 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. However, we can promote this single project manually to a full project for you.

Hi,

It's a good and very small module, but I don't see the point of such a module to be a separate live module just to add a library.

Thanks

Hi,

In its current state this module doesn't meet the minimum 120 lines of code to qualify as a project.

You say that this module integrates with libraries. Have you thought about using the latest recommended release and including 'hook_libraries_info' with a defined library version for this plugin. Have you thought about the features of the Libraries API for this. https://drupal.org/node/1342220

I think it's worth taking some time to expand the README.txt file and the project page for this module.

Is there any reason why you can't include some explanation alongside the link to the library resource on the project page? Is it worth telling users where to put it? 'sites/all/libraries' ? how to use it?

You've inserted a very long comment in 'hook_requirements' which shouldn't really include a github link, and this seems to be the full extent of your documentation for this module. The status report is not the place for installation instructions, but should complement them/provide status alerts.

Try to follow the best practice guidelines for creating a project https://drupal.org/node/7765.

The reasoning behind the regular expression used in hook_libraries_info() is to extract the version of a library and to ensure that each version is only used once (and make it availabe to others). The way you have written your code in hook_requirements() means that you have bypassed this. The status report should return the library version (plenty of examples there already). Ideally you should be looking at something in the order of:

if ($phase == 'runtime') {
    $library = libraries_detect('mylibrary');
    $requirements['mylibrary'] = array(
      'title' => $t('mymodule/mylibrary ... '),
    );
    if ($library['installed']) {
      $requirements['mylibrary']['value'] = $library['version'];
      $requirements['mylibrary']['severity'] = REQUIREMENT_OK;
    }
    else {
      $requirements['mylibrary']['value'] = $library['error'];
      $requirements['mylibrary']['description'] = $library['error message'];
      $requirements['mylibrary']['severity'] = REQUIREMENT_ERROR;
    }
  }

You don't seem to have addressed my previous comment on the length of the text in hook_requirements() and the presence of the github link.

You also need to look at $t = get_t(); which is missing in hook_requirements, along with its implementation in your text.

What about hook_help?

Have you considered all of the possibilities available to you when using the Libraries API, what about including the minified version as the default library for performance, while using the standard plugin file to extract the Library version?

You shouldn't really use hook_init() to load your library (loads on every bootstrap), try to do a bit of research. Have a look at hook_preprocess_page().

There is ample opportunity fo you to build this module out if you are willing, and unfortunately I think you need to address ALL of the above AND its length and feature-set before you set it back to NR.

• You should remove the 7.x branch, leave only 7.x-1.x.
• Why do you define JQUERY_TOUCH_SWIPE_URL? I don't see where it is used.
• You should implement a hook_uninstall() that removes all your variables with variable_del().
• Actually, should these two variables be merged? jquery_touch_swipe_select_minified_library and jquery_touch_swipe_jquery_touch_swipe_select_minified_library

Otherwise the module looks great!

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

Looks a lot better, thanks for taking on board some of the points mentioned.

Don't forget to set the issue back to 'needs review' after you've fixed 'needs_work'.

There are some issues remaining however:

File: jquery_touch_swipe.module

line 8: doc-comment refers to hook_init(), should be hook_preprocess_page().

line 91: you've created a parent menu item under 'admin/config' for this module, it should fall within a menu category such as 'User Interface' or 'Development' or ....

lines 114 & 134: By specifying the default state using variable_get() you will set the variable in the database if your return is 'return system_settings_form($form);'. You can bring your form submit message into the hook_form function, removing the need for the form_submit function. For this, use function jquery_touch_swipe_admin_form($form, &$form_state). This point is optional and just advice.

hook_permission();

Given the small size of this module, does it need a separate permission? imagine a website with 100 modules, do we really want 100 permissons to deal with. Presumably one of the existing admin permission will do in order to load a library. Optional, up to you.

File: jquery_touch_swipe.info

change to dependencies[] = libraries (>=2.x)

File: jquery_touch_swipe.install

Line 12: module_load_include(); "API: Loads a module include file." why do you need this? and you haven't specified a file to include.

You're still not returning the library version to the status report, its just blank if someone only installs the minified file. That's why I'd mentioned it before.

Other than that it's getting there.

Forgot to mention that the string in line 29 of .install file should be wrapped in $t ( from $t = get_t(); ), however this should really be $library_version['version']. This seems to be producing the error:

Notice: Undefined index: value in theme_status_report()

You should require both minified and standard to be installed in the library so that you can extract the version number, and satisfy your form options. Also you've not included the minified version in hook_libraries_info alongside the standard option. You could also warn the user if the library isn't found:

Example:

function jquery_touch_swipe_loaded() {
  if (($library = libraries_load('jquery_touch_swipe')) && !empty($library['loaded'])) {
    return TRUE;
  }
  elseif (user_access(' YOUR PERMISSION HERE ')) {
    // Alert the authorized user/administrator to the abscence of the library.
    drupal_set_message(t('The jQuery Touch Swipe Plugin Library could not be found. 
                          Please check the installation instructions and the <a href="@status">Status Report</a>.',
                          array('@status' => url('admin/reports/status'))), 'warning');
  }
}

You should specify what you've fixed in order to help the review process along. I see you've implemented just the 'corrections', and not a great deal more.

There are some errors returned by the pareview.sh script, you should really run this every time you make changes, see: http://pareview.sh/pareview/httpgitdrupalorgsandboxrafuel922045643git

#19

You're still not returning the library version to the status report, its just blank if someone only installs the minified file. That's why I'd mentioned it before.
line 29: this should really be $library_version['version']

And now I've mentioned it again.

#20

You should require both minified and standard to be installed in the library so that you can extract the version number, and satisfy your form options.
This seems to be producing the error:
Notice: Undefined index: value in theme_status_report()

Line 12: module_load_include(); "API: Loads a module include file." why do you need this?

You haven't explained why you need module_load_include() in your .install file.

You haven't addressed the issues quoted above, one of which is returning an error, both of which were not optional.

Although the review process can be difficult, you can make it easier by not ignoring points that are raised and by answering questions, and being informative aboout the changes you have made.

In addition, you were orignally advised that this module was too slim to qualify as a full project, and I have given you several pointers in previous comments as to how you could improve it and expand it. Also, The function I wrote for you in comment #20 could have been simply pasted in and would have expanded your module.

A negative issue with this module is that you have created a form which allows users to choose between standard and minified versions, this is entirely unnecessary, and you have been given plenty of pointers as to how this might be handled by the Libraries API. You need to address this.

You should install just the minified file and review the error, try to look at other modules for examples as to how you could 'require' both files to be installed.

Additional Observations:

You are recreating the functionality of the Libraries API by using libraries_get_libraries(), running it once in the libraries module and again in your module. You are looping through all possible library locations to find yours, however it's the job of the Libraries API to do this, that's why we use it.

If you look at the documentation you'll find that you can use the libraries API with very few functions, namely:

hook_libraries_info();
libraries_detect();
libraries_load();

Use the Devel module or Kint to print out libraries_detect() and you will see all of the values/booleans you can use, such as

$library['loaded'];
$library['installed'];
$library['version];
etc..

You can use these to satisfy all of the conditional logic you need, and you don't then need your function jquery_touch_swipe_get_library(), which tries to reinvent the wheel, and should be removed.

The way you've set up the install file is wrong, API: hook_requirements:

Module dependencies do not belong to these installation requirements, but should be defined in the module's .info file.

Or if not in the .info file using Libraries API.

You don't need to include any files in the .install file, you can use the values in the libraries_detect(); array to set your status alerts.

The solutions to the required rewrite of your module are actually much simpler than you have made them by not researching both the Drupal API, and Libraries API correctly, refer to examples used in other modules to learn what to use and when. There is also the examples for developers module.

I can only suggest that you rewrite this module, and only resubmit it when you are confident that it is close to being acceptable.

Please refer to the Additional Observations: in #22 above.

You still need hook_requirements in .install, you don't need the include file within this function.

Thanks for making the changes, I'll look at them tomorrow.t

Thank you for making some improvements. I don't see that you fully understand the Libraries API, because you are still using drupal_add_js()? You should remove those instances as 'libraries_load' does this for you. (so do you really need the form? and shouldn't the minified version be the default for performance?)

With regard to hook_requirements, I would suggest just using if ($phase == 'runtime') instead of the switch case, as you now alert the user on screen that the library is absent, so it seems blocking installation isn't necessary.

Other than that, its nearly ready. After you make the changes, please do a full test of, uninstall, installation from scratch, with and without library, to make sure it behaves as expected.

It is better than at the beginning, however there are still some issues:

The alternation between compressed and standard versions is not working, you can check the script tags in your HTML head after changing the form, to verify this. You need to correct the function jquery_touch_swipe_preprocess_page() and possible look into setting a variant callback function within hook_libraries_info();

The text of your form suggests that the minified version is incomplete, which is not the case:

Select yes for the minified version and no for the complete library

You haven't said why you think having an uncompressed version as the default is a good idea for performance, despite being asked in comment #26.

There still remains a question as to whether making the user perform what should be automatic functionality (switching between minified and uncompressed) adds anything of value.

I had hoped that we could set this to RTBC after the last round, but as issues remain, perhaps other reviewers can add something more.

Hi,

You should improve the language in the alert messages, particulary the error message on the form if a user enters an incorrect path. Currently there is no explanation as to whether this form accepts path aliases, or full paths, or node paths, with/without forward slashes.

You mention;

a new feature that allow users to enable the jQuery touch swipe library only on specific pages

Currently the library is loading on every page by default and entering paths in the admin form has no effect whatsoever. Even, if this was working, you haven't included any way to enable it on all pages, if required.

As this is the 30th comment so far on this project, I'm sorry to have to say that you do not seem to be properly checking your module's functionality after making changes. It is not the resposability of reviewers to provide the majority of the checks in comparison to the module developer.

You should ENSURE that the module works correctly in all aspects, if you want it to progress.

see above.

Your Git commit messages make it impossible to follow the changes you have made, can I suggest that you review:
Git; providing history
Best Practices

Commit messages such a single character "i" don't really help and many of the others ar nonsensical. I'm sorry to have to point out that it is this approach that has made this process a very lengthy one. The end result of having a module approved is that you gain Git commit privileges for your projects, so you need to show that you can manage repositories correctly.

Please add to your form that users can load the library on every page by setting 'node/*'. Remember that we don't know the level of experience each user of your module has. You can use the HTML break tag in your string to set a new line.

Also you haven't addressed my comment in #30 above regarding path aliases. Currently, an alias path such as 'blog/my-test-article' will trigger an error on the form. You could change your code to allow aliases, or you can explain that the raw node path is required.

A preferred method to add js files is to use 'Drupal.behaviors to wrap your js and use #attached (for example with hook_page_alter ) to add your JS files (if you can't add them using .info file). See: Drupal.behaviors

I didn't mention this earlier as it is more complex, and I will leave it as an option, but don't be surprised if it is mentioned by other reviewers. You could look at it though.

Please make sure everything is as expected, and I will make one final review once updated.

Sorry, just to add that you can use #attached to load your libraries (which include your js files).

The help path in jquery_touch_swipe module line 144 should be 'admin/help#jquery-touch-swipe'

correction, that should be 'admin/help#jquery_touch_swipe', and your help should now show up under the main help and under the modules page also.

Your help text is missing a full stop.

OK, I think that's it, thank you for your patience.

README.txt

Please take a moment to make your README.txt follow the guidelines for in-project documentation.

Otherwise, this looks great!

Thanks for your contribution, rafuel92!

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.