field slide show j360

• It appears you are working in the "master" branch, move from master to something like 7.x-1.x
• You have a lot of errors from Drupal coding standard validation.
  http://pareview.sh/pareview/httpgitdrupalorgsandboxcoderider2274229git

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.

Your git command is not in the correct format.

Like this: git clone http://git.drupal.org/sandbox/gwprod/2298383.git

It doesn't hurt to make things easier on volunteer reviewers.

Wrong page, sorry!

@coderider it is still wrong.

Your git command is still wrong.

You cannot use a command in the format username@git.drupal.org:sandbox/username/repo

It must be in the format http://git.drupal.org/sandbox/username/repo

1) PAReview is clear

2) Functionally: Module does not install via:

drush en field_slideshow_j360

Throws this error:

Module field_slide_show_j360 doesn't meet the requirements to be enabled.                                    [error]
Field slide show j360 require drupal_threesixty_slider. You must download drupal_threesixty_slider. Go to    [error]
download link
You must download drupal_threesixty_slider. Go to download link (Currently using drupal_threesixty_slider    [error]
Field slide show j360 require drupal_threesixty_slider)

You need to add a link to the library.

3) Several formatting issues in trigger.js that PAReview isn't detecting - they don't affect functionality, but an fyi that this something you should correct to make this file more uniform and readable.

It would be nice if you could integrate Drush so that required libraries would automatically be downloaded and installed, or at least create a message for drush users that is more meaningful. Example: http://cgit.drupalcode.org/modernizr/tree/drush/modernizr.drush.inc
But, I'm gonna reverse my original position that drush installation failing is a showstopper - even though it is the defacto drupal dev tool, requiring a module to work with it is not a requirement (at least as far as I know / have read).

Hi! A couple of helpful hints on the code:

• field_slide_show_j360.module:52
  You should use the matching Drupal theme functions, not raw HTML.

          'data' => '<img src="' . url('sites/default/files/' . file_uri_target($item['uri']), array('absolute' => TRUE)) . '" alt="' . $item['alt'] . '" />',

• field_slide_show_j360.install:22
  You use translation for a machine name 'drupal_threesixty_slider'. Is there any reason for that?
• field_slide_show_j360.install:59
  The $message argument of watchdog() function should be untranslated string. Drupal stores the log message in English, and automatically translates it at display time.
• field_slide_show_j360.install:46
  

    variable_set('field_slide_show_j360_installation_result', 'clean');
    if (variable_get('field_slide_show_j360_installation_result') != 'clean') {

  What is the reason to check the var you just set?

• js/trigger.js and theme/images_rotate.tpl.php:
  Incorrect syntax of @file doc block. Refer to https://www.drupal.org/node/1354#files
• theme/images_rotate.tpl.php:
  You should document available to the template variables.
• theme/images_rotate.tpl.php:7
  Do not use explicit HTML id attribute in template. IDs should be unique on the page, and 'container' ID can be used by another element or even by this template more than one time if it renders more than once on the page. Use class instead, or generate an unique id dynamically in preprocess/process function.

You can check and correct your code yourself and also improve your Drupal developing skills by using Coder Review module.

@coderider, thank you for contribution!

Here are some more code related tips for you.

1. In field_slide_show_j360.install file, at line 51 you have used following line.

variable_set('field_slide_show_j360_installation_result', 'dirty');

Not sure what is the benefit of variable_set() here, when you already set the message(drupal_set_message).

2. In field_slide_show_j360.module file, at line 67&69 return $element; is defined twice but you can defined it only once. You can remove 67 line return statement.

3. In same file at line 127, you are using following code:

if (isset($map[$key]['map']) && isset($map[$key]['map'][$value])) {
        $value = $map[$key]['map'][$value];
      }
      $summary[] = $title . ': ' . $value;

It should be:

if (isset($map[$key]['map'][$value])) {
   $value = $map[$key]['map'][$value];
   $summary[] = $title . ': ' . $value;
}

I think these need to be fixed before moving to RTBC.

Thanks again!

Removing review bonus tag, you have not done all manual reviews, you just repeated 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.

@coderider, some minor suggestions.

1. Remove following unused code from .install file, if it is not required.

/**
 * Implements hook_install().
 */
function field_slide_show_j360_install() {
  // drupal_set_message('field slide show j360.');
}

2.

You can remove 67 line return statement.

its just inside and outside of the condition

If it comes inside if statement then it would also come outsides this if statement and $element will return through external return. There is no need to use two return statement here, do remove line 67 return statement.

3. hook_help() is missing in your module.

4. spelling mistake in following line in README.txt file.

now go for display settings and select "Xulu 3D image rotator"
for you field and set height and width as your theme requiered.

As there is no blocker from my side, so not moving to "Need Work" but above issues are required to fix before moving to RTBC.

Thank you for your contribution.

Automated Review still comes back clean even after all the changes I can see you made :)

Manual Review:

Code-Related

1. (*) In hook_libraries_info(). I see you set the library version directly rather than providing a version callback. According to the documentation for the libraries module "version callback" is highly recommended and "version" should only be used if there is no way to automatically detect the version. Since I see you are the author of the library I suggest adding the version to the library such that you can detect it programatically.
2. You code is quite condensed and does not contain very many in-line comments. It's recommended that you have a balanced number of comments explaining your code and some well-placed empty lines would greatly improve readability.
3. On line 19 of images_rotate.tpl.php you don't adhere to the max 80 character length coding standard. This could be made more readable by defining a php array near the top of the file containing all the classes for the
   element, then on line 19 you could simply have an implode call.
4. (*) You README is quite sparse. Specifically, you are missing an introduction (required) & Requirements (required). Also, in the installation section you should include instructions for including the library (perhaps just a link to drupal.org documentation on installing libraries). See the README Template for additional help and examples.

Installation-Related

(*) When I git cloned the library you linked to it created both the directory & filename (drupal-threesixty-slider.js) using dashes instead of underscores. This blocked installation of your module since the file wasn't recognized by your module. Making me change the folder name is fine but making your users rename library files to get them detected by your module seems like a bit much to expect. Additionally, even once I had renamed both the folder and the file, installation still failed due to requirements.

Note, I consider (*) comments above to be major whereas the others are just suggestions :)

Automated Review

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

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

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/README.md

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

The field settings are XSS prone. Even though you have a length on these, they are alterable. The width and height go straight from input to output
in the summary. Dont' do the loop. Just build up the summary manually. Use placeholders with t() for the settings (this will plain it).

These also go straight to output in the template. Instead of passing all of the settings to the theme function, break them out into separate keys. Then check_plain()
them.

See https://www.drupal.org/node/28984 for more info.

Coding style & Drupal API usage

It's probably premature to have version tags in the repo.

Why do you have a behavior that sets an explicit onload? You need to wait for the images to load in? Will this be AJAX friendly? Comment needed.

Your JS needs to use the context that was passed in via the behavior.

There is no hook_install_error in Drupal 7.

There is no hook_get_version(). This is the version callback for the hook_libraries_info().

In field_slide_show_j360_field_formatter_view(), using #attached is preferred over directly using drupal_add_css() and drupal_add_js().

You don't need to build up markup in field_slide_show_j360_field_formatter_view(). Build up a renderable element and put that into the element.

Your template, CSS, and JS files should all be namespaced to your module.

(*) I got a JS error on line 48 when I tried to test this.

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.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  FILE: /home/klausi/pareview_temp/field_slide_show_j360.module
  --------------------------------------------------------------------------------
  FOUND 4 ERRORS AFFECTING 3 LINES
  --------------------------------------------------------------------------------
   92 | ERROR | Additional blank lines found at end of doc comment
   93 | ERROR | Type hint "array" missing for $library
   93 | ERROR | Type hint "array" missing for $options
   99 | ERROR | Function return type is not void, but function is returning void
      |       | here
  --------------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

• there is still the #markup used in field_slide_show_j360_field_formatter_view() instead of just putting #theme and the variables in there.

But otherwise looks RTBC to me. Assigning to mpdonadio as he might have time to take a final look at this.

Manual review of 7.x-1.x 184489a.

My security issues from #40 look good now, and I read the diff from my last review and went through it again.

I still agree the minor recommendations that weren't done.

The summary in field_slide_show_j360_field_formatter_settings_summary() should use t() w/ placeholders. Right now, the strings are untranslated. t() w/ @ pr % placeholders will plain for you.

You have some spelling errors here and there. Some IDEs will spellcheck for you (PHPStorm does).

Not seeing anything major to prevent approval.

Thanks for your contribution, coderider!

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.