[D8] Superslides Fullscreen Caption Slider

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

Fixed the git clone URL in the issue summary for non-maintainer users.

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.

I've enabled the module, create a view with title and image fields , set the format at "Superslides" , enable the block display .. and when i go to a page where this block is displayed i got "wsod"

Hi kamdanishit,

I have tested this module with "Bartik 8.1.1" theme. Please check "view_configration.png" for my view settings. Correct me if i am missing anything, Few points are :

1. Module is not working for me. Its giving js error. Please check "js_error.png"
2. Please clear all point on http://pareview.sh/pareview/httpgitdrupalorgsandboxkamdanishit2735609git
3. Follow the Readme template for the readme.txt https://www.drupal.org/node/2181737

Resetting priority to "Normal", as per: https://www.drupal.org/node/539608

Manual Review

1. Basic application checks

   1.1 Ensure your application contains a repository and project page link.

   [Yes: Follows]

   1.2 Ensure your project is not duplication.

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

2. Basic repository checks

   2.1 Ensure the repository actually contains code.

   [Yes]

3. Licensing checks

   3.1 Ensure the repository does not contain a 'LICENSE.txt' file.

   [Yes: Follows] the licensing requirements.

   3.2 Ensure the repository does not contain any 3rd party (non-GPL) code.

   [Yes: Follows] the guidelines for 3rd party assets/code.

4. Documentation checks

   4.1 Ensure the project page contains detailed information.

   [Yes: Follows] the guidelines for in-project documentation

   4.2 Ensure the repository contains a detailed README.txt.

   [Yes] But does not followed the guidelines for README Template.

   4.3 Ensure the code contains a well-balanced amount of inline-comments.

   [Yes].

5. Coding standards and style

   5.1 Run an automated review and ensure there are no major issues.

   [No: Automated review has not passed. Please check http://pareview.sh/pareview/httpsgitdrupalorgsandboxkamdanishit2735609git ]

Working link to PAreview:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxkamdanishit2735609git

@ashwin.shaharkar: look like you forgot to change the status. Is this now RTBC after your review or are there application blockers left and this should be "needs work"?

Hello @naveenvalecha,

Automated review has not passed for this project, that's why I did not mark this to RTBC.

Thank you,

Automated review is not the blocker, anything else that you found.

You still haven't got the right default branch. Neither "bugfix" nor "8.1.x" are correct. You need to change this to "8.x-1.x". IMHO, this error should block promotion (however, it is very easy to fix).

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.

And while code style errors are not blockers (as pointed out in #12), projects with a lot of trivial format errors are just harder to review manually. Some of the volunteer reviewers (including me) prefer to pick "lower hanging fruits" when we select what projects to manually review.

Fixed the git clone URL in the issue summary for non-maintainer users.

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

Manual Review

1. Basic application checks
1.1 Ensure your application contains a repository and project page link.
[Yes: Follows]
1.2 Ensure your project is not duplication.
[Yes: Does not cause] module duplication and/or fragmentation.
2. Basic repository checks
2.1 Ensure the repository actually contains code.
[Yes]
3. Licensing checks
3.1 Ensure the repository does not contain a 'LICENSE.txt' file.
[Yes: Follows] the licensing requirements.
3.2 Ensure the repository does not contain any 3rd party (non-GPL) code.
[Yes: Follows] the guidelines for 3rd party assets/code.
4. Documentation checks
4.1 Ensure the project page contains detailed information.
[Yes: Follows] the guidelines for in-project documentation
4.2 Ensure the repository contains a detailed README.txt.
[Yes] But does not followed the guidelines for README Template.
4.3 Ensure the code contains a well-balanced amount of inline-comments.
[Yes].
5. Coding standards and style
5.1 Run an automated review and ensure there are no major issues.
[No: Automated review has not passed.]
http://pareview.sh/pareview/httpsgitdrupalorgsandboxkamdanishit2735609git

3. I'm almost certain that the MIT license for animate.css (superslides_fullscreen_caption_slider/style/ainmate.css) is ok but I don't know if an exception needs to be filed as per: https://www.drupal.org/node/422996#comment-9654015. @gisle, could you weigh in on that?
4. Recommend adding the library mentioned in the installation portion of the README to a newly added REQUIREMENTS section - https://github.com/nicinabox/superslides
5. I went ahead and rolled a patch for the pareview issues here: https://www.drupal.org/node/2760895. None of the issues I saw would be considered release blockers.

Ran the module through it's paces as well on a fresh D8 install. Works as expected.

One other note for 5.1 project page (https://www.drupal.org/node/997024):

You should mention some slideshow module alternatives on your project page. I think you've already done a decent job at differentiating this module in the description but there are quite a few other paths. e.g. https://www.drupal.org/project/views_slideshow, https://www.drupal.org/project/jssor, etc.

In #21, Jon Reid wrote:

I'm almost certain that the MIT license for animate.css […] is ok but I don't know if an exception needs to be filed as per: https://www.drupal.org/node/422996#comment-9654015. @gisle, could you weigh in on that?

An exception need to be granted by the LWG for any third party materials in the repo. In this case, no exception request has been made.

The MIT license is one of the licenses we allow, however, as per 3rd party libraries and content on Drupal.org, exceptions can be made if the 3rd party library/content:

1. had to be modified to work with Drupal, and the modifications were not accepted by the original author;
2. is generally difficult to find, or the specific version needed is hard to find;
3. is no longer maintained by the original author;
4. has a license that allows us to distribute it under GPLv2+.

In this case only #4 seems to be the case, so if a exception request was made, it would be denied.

Also, as noted in #22 by Jon Reid, there are plenty of other slideshow bridge modules in Drupal-land. Our users need to be informed about possible functional overlap. This should be made is section with the heading "Similar projects and how they are different" on the project's project page that:

1. acknowledges the existence of similar projects; and
2. briefly explain how they are different.

This project does not satisfy the criteria for 3rd party contents, and the project page fails to inform about alternative projects. It must go back to Needs work.

Hi,

please don't change the status before fixed the issues. please fixed pareview issues:

http://pareview.sh/pareview/httpsgitdrupalorgsandboxkamdanishit2735609git

Thanks

Minor coding standard problems are surely not application blockers, please do a real manual review.

Hi kamdanishit,

I did a manual review and please see my comments below:

Automated Review

Fix the issues mention in automated review.

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

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

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No: List of security issues identified. Refer Writing secure code

Coding style & Drupal API usage

1. (*) Found security vulnerabilities in your code.
   All user entered text should be pass through SafeMarkup::checkPlain()/Xss::filter() before render back in to the browser. In slider you are displaying field contents without sanitization. Add sanitization user inpu fields. Check SuperSlider.php render() function.

   	  for ($i = 0; $i < count($this->view->result); $i++) {
               for ($j = 0; $j < count($field_labels); $j++) {
                   $field_item_list = $this->view->result[$i]->_entity->get($field_labels[$j]);
                   $field_type = $field_item_list->getFieldDefinition()->getType();
   
                   $renderdata[$i][$field_type] = array('value' => $this->view->style_plugin->getField($i, $field_labels[$j]), 
                                                        'label' => $field_labels[$j], 
                                                        'options' => $this->options[$field_labels[$j]]);
               }
           }
   	  

2. (+) As this module is fully depends on two external libraries make use of hook_requirements() to check the availability of libraries.
3. All user facing text should be pass through translate function. In your code found mixed use of t() and $this->t(). Use $this->t() inside classes.
4. Give a meaningful name to your drupal javascript behaviour. Found
   

   	  Drupal.behaviors.themeExample = {
           attach: function (context, settings) {
               // jQuery once ensures that code does not run after an AJAX or other function that calls Drupal.attachBehaviors().
               $('body').once('themeExample').each(
   			...
   	  

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.

Thanks,
ARUN AK

@ARUN AK: I could not reproduce the XSS security issue. Drupal 8 auto-escapes the data from the view, so when I tried to enter <script>alert('XSS');</script> as node title to see if it triggers XSS in the view it was escaped as expected. Maybe I missed something? Can you write down the steps to exploit this so that we can test the XSS vulnerability?

Removing the security tag in the meantime.

@kamdanishit: it seems that the online pareview.sh service is broken right now and does not show all the Coding Standard errors in SuperSlides.php. Can you install Coder and check that yourself? https://www.drupal.org/project/coder
I'm attaching the output of Coder, please fix the coding standards in the meantime so that reviewing is easier. Thanks!

manual review:
Notice: Undefined index: field_image in Drupal\superslides\Plugin\views\style\SuperSlides->render() (line 240 of modules/superslides/src/Plugin/views/style/SuperSlides.php). Make sure to enable PHP notices in your dev environment.

@klausi, When I test I added node body field which accepting full html in slideshow views. So the alert was coming during slideshow. But it can be managed by selecting proper Text format filter in field configuration.

If a user can select the text format "full HTML" then of course they are allowed to put in any HTML tag, even script tags. That is the purpose of the full HTML text format, never give that to untrusted users. See https://www.drupal.org/node/224921 and https://www.drupal.org/node/1884352

So this is not a vulnerability in this module, anything else that you found?

There is some coding standard errors to fix :

Review of the 8.x-1.x branch (commit 7613bbd):

• ESLint has found some issues with your code (please check the JavaScript coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: ...upal-7-pareview/pareview_temp/src/Plugin/views/style/SuperSlides.php
  --------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------
   64 | WARNING | There must be no blank line following an inline comment
  --------------------------------------------------------------------------
  
  Time: 38ms; Memory: 6Mb
  

• 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.

/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 1, col 2, Error - Use the function form of 'use strict'. (strict)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 2, col 5, Error - Expected indentation of 2 space characters but found 4. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 3, col 9, Error - Expected indentation of 6 space characters but found 8. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 5, col 13, Error - Expected indentation of 10 space characters but found 12. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 8, col 21, Error - Expected indentation of 18 space characters but found 20. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 10, col 21, Error - Expected indentation of 18 space characters but found 20. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 12, col 25, Error - Expected indentation of 22 space characters but found 24. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 13, col 29, Error - Expected indentation of 26 space characters but found 28. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 16, col 25, Error - Expected indentation of 22 space characters but found 24. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 18, col 25, Error - Expected indentation of 22 space characters but found 24. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 19, col 29, Error - Expected indentation of 26 space characters but found 28. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 21, col 33, Error - Expected indentation of 30 space characters but found 32. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 22, col 33, Error - Expected indentation of 30 space characters but found 32. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 23, col 33, Error - Expected indentation of 30 space characters but found 32. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 24, col 9, Error - Expected indentation of 30 space characters but found 0. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 25, col 1, Error - Trailing spaces not allowed. (no-trailing-spaces)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 27, col 33, Error - Expected indentation of 30 space characters but found 32. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 29, col 37, Error - Expected indentation of 34 space characters but found 36. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 31, col 37, Error - Expected indentation of 34 space characters but found 36. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 31, col 37, Error - Move function declaration to function body root. (no-inner-declarations)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 31, col 37, Error - Don't make functions within a loop (no-loop-func)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 32, col 41, Error - Expected indentation of 38 space characters but found 40. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 34, col 49, Error - Expected indentation of 46 space characters but found 48. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 39, col 37, Error - Expected indentation of 34 space characters but found 36. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 42, col 9, Error - Expected indentation of 30 space characters but found 0. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 42, col 9, Error - Expected space(s) after "if". (keyword-spacing)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 42, col 31, Error - Infix operators must be spaced. (space-infix-ops)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 42, col 39, Error - Missing space before opening brace. (space-before-blocks)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 43, col 34, Error - Expected indentation of 2 space characters but found 33. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 44, col 38, Error - Expected indentation of 35 space characters but found 37. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 45, col 42, Error - Expected indentation of 37 space characters but found 41. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 46, col 11, Error - Expected indentation of 37 space characters but found 0. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 47, col 38, Error - Expected indentation of 35 space characters but found 37. (indent)
/var/www/drupal-7-pareview/pareview_temp/js/superslides-custom.js: line 50, col 30, Error - Unnecessary semicolon. (no-extra-semi)

34 problems

Source: http://pareview.sh/ - PAReview.sh online service

There is some coding standard errors to fix :

EDIT: removed long pareview.sh dump.

Source: http://pareview.sh/ - PAReview.sh online service

manual review:

1. There is a confusing 8.1.x git branch. Is that one still needed? If not it should be deleted so that it does not get confused with 8.x-1.x.
2. superslides.info.yml: The libraries key does not exist for module info files, so this line should be removed.
3. SuperSlides.php: "'#title' => 'Global',": all user facing text must run through $this->t() for translation.
4. Same for the option groups you are using, those also look like user facing categories that should be translated.
5. superslides-custom.js: the indentation level in the .ech() looks wrong, should always be 2 spaces.

Otherwise looks good to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to er.pushpinderrana as he might have time to take a final look at this.

Since this was RTBC already and the comments seem to be addressed, so...

Thanks for your contribution, Nishit Kamdar!

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.