[D7] Viewer JS

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

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.

Please fix the issues from automatic test:
http://pareview.sh/pareview/httpgitdrupalorgsandboxmouhammed2396271

Regards.

Looks good, thanks.

Please change above description to list
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/mouhammed/2396271.git viewerjs
as the git address so that people other than yourself can clone.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

3rd party assets/code

No: Although viwerjs library is done correctly, fontello is included in the module. Icon API and Fontello modules exist https://www.drupal.org/project/fontello as well as simply adding Fontello to sites/all/libraries. Is fontello required by viewerjs or is it something added by this module?.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template. Installation is light on a couple of steps, namely that a file field must be created or existing file field used.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements. All user access and permissions handled by Field API.

Coding style & Drupal API usage

1. (*) libraries_get_libraries from within the viewerjs.install file hook_requirements crashes as, despite the libraries module being a dependency, if it's not enabled when enabling viewerjs the fatal error "Call to undefined function libraries_get_libraries()..." is received before any dependency resolution can begin. In general modules powered by libraries allow themselves to be enabled without the library being installed and present warning messages to the administrator with a link to download the library.
2. (+) js/viewerjs.js: In the Javascript file, wrap all the code in (function ($) { })(jQuery); and use $ notation to make reference to jQuery. See https://www.drupal.org/node/304258
3. (+) js/viewerjs.js: Use Drupal behaviors instead of document ready. This will allow your code to work with AJAX requests and be overridden by glue code modules. See https://www.drupal.org/node/304258
4. (+) viewerjs_field_formatter_view is difficult to follow and impossible for developers to override effectively if they wish to do their own templating. What would be great is if viewerjs_field_formatter_view dispatched to the Drupal Theme API with template files. That way if I, as a developer, wanted to change the way the download and view links worked (put them at the bottom, change their icons etc) I could simply override the tpl file in my theme or preprocess it.

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.

This review uses the Project Application Review Template.

Automated Review

2 Best practice issues identified by pareview.sh / drupalcs / coder. Results can be ignored

Manual Review

Licensing

No: Does not follow the licensing requirements. We require that all files (PHP, JavaScript, images, Flash, etc.) hosted on Drupal.org be under the GPL. If it's in Git, then it is under the same license.

1. Found font file in 4 formats with Creative commons.
2. Found 2 images that may also not be GPL compatible.

3rd party assets/code

No: Does not follow the guidelines for 3rd party assets/code. Fontello files (font)needs to be removed from the module and has to go into libraries folder. Possible integration with Fontello https://www.drupal.org/project/fontello module suggested.

README.txt/README.md

No: Does not follow the guidelines for in-project documentation and/or the README Template. Readme file doesnt have any description. The README-file should repeat the synopsis on the project page.

Coding style & Drupal API usage

List of identified issues in no particular order.

1. (*) Fatal error: Call to undefined function viewerjs_get_viewerjs_path() in /var/www/modules_export/drupal2/sites/all/modules/viewerjs/viewerjs.install on line 56

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.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

I've tested your module and I must say that it works quite well. However, I may found a bug:

When in formatter options, I select option "Nothing" at select "Preview type" it does the same as when I select option "New window". Could you check this out? Maybe in case the users select "Nothing" the "Preview" link should dissappear.

Also, for the code side, all seems ok to me now that you use the libraries module. However, you still have to solve the issues shown by pareview.sh. The errors shown there are simple So I think you can fix all of them quickly.

Congratulations for this module!

Assigning to myself for next review.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. Yes, found few issues.

Review of the 7.x-1.x branch (commit 9770980):

Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
FILE: /var/www/drupal-7-pareview/pareview_temp/README.md
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------------
15 | WARNING | Line exceeds 80 characters; contains 84 characters
16 | WARNING | Line exceeds 80 characters; contains 153 characters
17 | WARNING | Line exceeds 80 characters; contains 129 characters
--------------------------------------------------------------------------------

Time: 206ms; Memory: 9.75Mb

DrupalPractice has found some issues with your code, but could be false positives.
FILE: /var/www/drupal-7-pareview/pareview_temp/viewerjs.module
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
--------------------------------------------------------------------------------
232 | WARNING | #options values usually have to run through t() for
| | translation
250 | WARNING | #options values usually have to run through t() for
| | translation
--------------------------------------------------------------------------------

Manual Review

viewerjs_help(): why you make this function so complicated. Also why filter_xss_admin() and check_plain() is require here? You are not printing user provided text here but only the trusted README file?

viewerjs_field_formatter_view(): In general, using #attached is preferred over drupal_add_css, drupal_add_js, and drupal_add_libray.

viewerjs_thumbnail_file(): $base_url is almost never needed directly. Use url() to make an absolute URL if needed.

But that are not critical application blockers, otherwise I think this is RTBC.

Assigning to klausi for a second look if he has time.

Git errors:

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

Review of the 7.x-1.x branch (commit 9770980):

1. found the same problems as er.pushpinderrana, please fix them.
2. viewerjs_get_viewerjs_path(): no need for module_exists('libraries') since you depend on it in the info file and it will always be there.

But that are not critical application blockers, so ...

Thanks for your contribution, mouhammed!

I updated your account so you can 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 stay 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.