[D7] Responsive Slideshow

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.

Here is my bit:

1. in the Readme file, line 41: "The the image style dimensions of the image can be" has a double The.

2. If it is responsive, is not clear how to setup, or if it is possible, the image styles for different breakpoints.

3. responsive_slideshow_build_string, returns a string with HTML code, in general that is not a good practice. I advise to change this to a template and preprocess function and some suggestion so themers can work better with it.

Hi viennethaw, thanks for the contribution. I took a quick look and it seems that you could use the https://www.drupal.org/node/222158, system_settings_form() method instead of handle the responsive_slideshow_settings_submit() by yourself.

This may no needed

/**
 * Submit handler for responsive_slideshow_settings form.
 *
 * @see responsive_slideshow_settings()
 */
function responsive_slideshow_settings_submit($form, &$form_state) {
  if (!empty($form_state['values']['responsive_slideshow_no_of_slides'])) {
    variable_set('responsive_slideshow_no_of_slides', $form_state['values']['responsive_slideshow_no_of_slides']);
  }
  if (!empty($form_state['values']['responsive_slideshow_description_length'])) {
    variable_set('responsive_slideshow_description_length', $form_state['values']['responsive_slideshow_description_length']);
  }
  if (!empty($form_state['values']['responsive_slideshow_interval'])) {
    variable_set('responsive_slideshow_interval', $form_state['values']['responsive_slideshow_interval']);
  }
  drupal_set_message(t('The configurations have been saved.'));
}

Also this is not necessary but you could provide some initial default values for variables you are using

 variable_get('responsive_slideshow_no_of_slides', DEFAULT_VALUE)

Regards

Default value not set of variable.Value should be contain all variable for default selection.
#default_value' => variable_get('responsive_slideshow_description_length')
#default_value' => variable_get('responsive_slideshow_interval'),

it should be look like this:
#default_value' => variable_get('responsive_slideshow_description_length',NULL)
#default_value' => variable_get('responsive_slideshow_interval',NULL),

Hi vineethaw,

I found only one minor issue.
responsive_slideshow.info has typo in the description/misspelling of bootstrap: "Module for getting responsive slideshow in bootsrap theme."

Other than that, your code looks good to me.

A typo is surely not an application blocker, anything else that you found or should this be RTBC instead?

Thanks for contribution @vineethaw

.carousel-indicators {
    position: absolute;
    top: 60px;
  }

Remove position: absolute; from responsive_slideshow.css file .Because its overwritten if this module is use bootstrap theme then its fetch
style for .carousel-indicators from bootstrap theme .

Other then that , your code is working good for me.

Hi vineethaw,
I see your code it ruining well,
inotice that in .install file you use

/**
* Implements hook_uninstall().
*/
function responsive_slideshow_uninstall() {
// Machine name of the content type.
$name = 'responsive_slideshow';
// Gather all job nodes created.
$sql = 'SELECT nid FROM {node} n WHERE n.type = :type';
$result = db_query($sql, array(':type' => $name));
$nids = array();
foreach ($result as $row) {
$nids[] = $row->nid;
}

friend you can not use the drupal7 query standard it is look like the way of write in query in drupal6

Code looks good - followed the README and successfully created a slideshow

A few suggestions (but not critical):

• responsive_slideshow_block_view() – Consider using a renderable array for $block['content'] instead of calling theme() directly. Could also break the array onto multiple lines.
• responsive_slideshow_form_alter() – hook_form_alter() arguments should be &$form, &$form_state, $form_id. You could maybe also use responsive_slideshow_settings_validate() instead.

Following up on #20, you could use EntityFieldQuery - see https://www.drupal.org/node/1343708. Also, as this is tied to the responsive_slideshow content type, you could also introduce some caching around this query and invalidate the cache when saving a responsive_slideshow node.

@davidgrayston, @prajaankit,

This should be on "Need Work" or "RTBC" ?

Apologies, my second point was incorrect – I should have said the callback arguments don't match those in the documentation: https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

responsive_slideshow_numeric_validation($form_state)

Should be

responsive_slideshow_numeric_validation($form, &$form_state)

This will probably mean the use of $form_state in the function will need to be changed to $form

Happy to change to RTBC once this has been looked at (unless others feel this isn't a blocking issue)

Great! There's just one last quick thing - the automated review has highlighted that the default branch hasn't been set - see http://pareview.sh/pareview/httpgitdrupalorgsandboxvineethaw2478405git

I can test your changes today and will update the status

I think this is mostly done

The module is still working with the recent changes so happy to leave as RTBC

I spotted a small problem with the description being printed twice:
$sliders[$key]['description'] .= $sliders[$key]['description'] . '... ';
Should be:
$sliders[$key]['description'] .= '... ';

Worth fixing now but I don't think this should block the application so leaving status as RTBC

manual review:

1. Looks like you created the 50th slideshow module on drupal.org, see https://www.drupal.org/node/418616 . Why can't you use one of the existing modules? What are the differences to the existing modules? Please add that to the project page. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition, so I would rather help improve one of the existing projects instead of launching yet another one.
2. responsive_slideshow.info: why do you need the CSS on every single page request? I think you should only add it when the slideshow is actually displayed on a page?
3. responsive_slideshow_install(): do not juggle with module weights in the system table as that is unreliable. Use hook_module_implements_alter() instead if you need to run before/after a specific hook. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
4. responsive_slideshow_enable(): do not overwrite settings when the module is enabled. If I disable the module and re-enable it again then the settings should be the same. You can make use of default values with varaible_get() in responsive_slideshow_settings() for example anyway, so I would just remove that hook.
5. responsive_slideshow_uninstall(): do not call db_selete() directly on the node table here, you should gather all IDs of the nodes you wish to delete and call node_delete_multiple() instead. See https://api.drupal.org/api/drupal/modules!node!node.module/function/node...
6. responsive_slideshow_homepage(): do not call node_load() on every single row, use node_load_multiple() instead. That way you only make one query instead one per node. See https://api.drupal.org/api/drupal/modules!node!node.module/function/node...
7. responsive_slideshow_homepage(): you forgot the node access tag on your query against the node table here, see https://www.drupal.org/node/93737 . I would argue that this is not a critical security issue since the intent is probably to always have all the published slideshow node visible, but this should be fixed anyway.
8. responsive_slideshow_form_alter(): if you are targeting only one form you should use hook_form_FORM_ID_alter() instead. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
9. There is another security vulnerability in this module and as part of our git admin training I'm assigning this to Manjit.Singh so that he can take a look. If he doesn't get around to post details here in one week I will follow-up with exploit details.

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

The README contents could be sanitized using check_plain() in hook_help()

nope, since the README content is not user provided input it does not have to be sanitized.

I've taken another look at the code and can see that the image title/alt attributes aren't escaped

You can execute JavaScript on the page by entering something like this as the node title, image alt or image title:
"><script>alert('Hello!')</script><"

This also happens to description if you have views module enabled

$sliders[$key]['description'] has tags stripped, but then $slide_desc is used instead

Reproduce by using this as the teaser or description: <script>alert('Hello!')</script>

Good catch davidgrayston ;)

1. $sliders[$key]['title'] is looks good now..

2. $sliders[$key]['description'] is still a vulnerable. Please check https://www.drupal.org/node/28984 and https://api.drupal.org/api/drupal/includes%21common.inc/group/sanitizati...

Other than that code looks good to me.. Let's wait for Klausi's feedback ;)

Hello,

README.txt/README.md
NO: Follows README Template.
https://www.drupal.org/node/2181737

sorry, for me that not start work, only show nodes at home page like nodes list, and without any "Responsive Slideshow"

Also why you need set image count in module ? Why not take that from database ?

Thanks

Hello vineethaw,

For me all continue not work, please check http://ordasvit.com/drupal736test/
Access details I sent to you email.

Also not see change in README, please exactly like https://www.drupal.org/node/2181737

I not understand: why you need set image count in module ? Why not take that from database ?

"Enter the interval between the slides in the carousel." - what unit user must use ?

Thanks

Hello vineethaw,

I special install to http://ordasvit.com/drupal736test/ responsive theme. And that theme has bootstrap. Your module continue not work.
Responsive you may check:https://www.responsinator.com/

Thanks

Hello,

Already we had a review and fixes with 10 Drupal community members,

I am sorry, but Drupal create some rules for review, And I ask from you all only by these rules.

I not see change in README, please exactly like "README Template" https://www.drupal.org/node/2181737 don't fix

As we have mentioned in the above comments, you have to enable the Drupal Bootstrap

I not see in your module Requirements - nothing about some exactly theme. If you write "Bootstrap theme" - that or all themes what support Bootstrap or some exactly theme with link to it. But as me seem - module created for some exactly theme - that bad module. Module must support as many themes as possible.

Thanks,

README improvements and a link to the required bootstrap theme are surely not application blockers, anything else that you found or should this be RTBC instead?

Hello,

I am sorry, but if module has requirements, it must show all requirements details in README.

Also as me seem for module very bad - if it work only for one theme.

Module incorrect change size for some devices at time Responsible
http://www.responsinator.com/?url=ordasvit.com%2Fdrupal736test%2F

it incorrect work for:
iPhone 5 portrait · width: 320px
iPhone 5 landscape · width: 568px
iPhone 6 portrait · width: 375px
iPhone 6 Plump portrait · width: 414px
Android portrait · width: 240px
Android landscape · width: 320px
and other width .....

Thanks

Hello vineethaw,

If you recheck your sent images, you will see this "responsive Slideshow" - responsive not at all requirement views!!

Why you show some image with text, some other without text?
If text broke sliders, please or fix or remove text from show.
Please correct positions of arrows .

On http://ordasvit.com/drupal736test/ - I special created test host(where all may see how real that look and work), and now on it installed your Bootstrap theme from drupal.org and your module. And all looks not good

Thanks

1. No need to use quotes in description and package in .info file

It should be
description = Module for getting responsive slideshow in bootstrap theme.
package = Bootstrap

2. It would be better if the *.info file look like this:
name = Responsive Slideshow
description = Module for getting responsive slideshow in bootstrap theme.
package = Bootstrap
core = 7.x
configure = admin/config/user-interface/responsive_slideshow

dependencies[] = image
dependencies[] = jquery_update

manual review:

1. You still haven't addressed the project page with the fact that this is yet another slideshow module. What are the differences to the existing modules? Please add that to the project page.
2. "$sliders[$key]['title'] = substr(strip_tags($slide_node->title), 0, 90);": don't use substr() here as that will not work for all UTF-8 characters. Use truncate_utf8() instead, see https://api.drupal.org/api/drupal/includes!unicode.inc/function/truncate...
3. "strip_tags($slide_node->title)": do not use strip_tags() on the node title, use check_plain() instead. See https://www.drupal.org/node/28984
4. views_trim_text(): I think that call to Views codde should not be needed anymore of you switch to truncate_utf8().
5. responsive_slideshow_homepage(): why do you use menu_get_item() to check for access here? You should just use node_access() instead?
6. responsive_slideshow_module_implements_alter(): this should have a comment why you are doing this and before/after what other hook implementation you have to run.
7. responsive_slideshow_form_responsive_slideshow_settings_alter(): why do you alter your own form? You can just add the validation clabback in responsive_slideshow_settings() and remove this function?
8. There is still a security vulnerability in the code. Assigning back to Manjit.Singh so that he can take a look. If he does not find anything I'm going to post the details about the vulnerability in one week.

Please update this as per instructions

Sorry for the delay.

responsive_slideshow_homepage(): this is vulnerable to XSS exploits. If I enter "><script>alert('title');</script> as node title and that node is displayed in a slideshow I will get a nasty javascript popup. You need to sanitize user provided text before printing to HTML. Make sure to read https://www.drupal.org/node/28984 again.

This is currently a security blocker.

Thanks @vineethaw for the contribution. title is now properly sanitize user provided text. But still responsive_slideshow_homepage(): is vulnerable to XSS exploits. If I enter

as a node description, then node is displayed a nasty javascript popup. Please check screencast for the more information.

Please use check_markup() for this rather than check_plain().

When we should use textarea we are usually use check_markup() to sanitize the use provided data.

The Git commits are not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 and https://www.drupal.org/node/1051722

manual review:

• responsive_slideshow_module_implements_alter(): why do you change the order of hooks? In what direction? Please add a comment.

But otherwise looks good to me now.

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

Sorry for coming late.I was on tour.

The Git commits are not connected to of both the module contributors(vineethaw, sajiniantony) You need to specify an email address. See https://www.drupal.org/node/1022156 and https://www.drupal.org/node/1051722
if your commits are associated with different emails then file an issue in the drupal infra to update your email in the commits https://www.drupal.org/project/issues/infrastructure?categories=All

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

Manual Review :

1. Please use good commit messages. See https://www.drupal.org/node/52287
2. Use the project page template for project page https://www.drupal.org/node/2190239
3. Specify the default values of variables in variable_get http://cgit.drupalcode.org/sandbox-vineethaw-2478405/tree/responsive-sli...
4. Move the responsive_slideshow_settings function to a seperate .admin.inc file It will give some performance benifits.
5. Use the default values with variables. http://cgit.drupalcode.org/sandbox-vineethaw-2478405/tree/responsive_sli... , http://cgit.drupalcode.org/sandbox-vineethaw-2478405/tree/responsive_sli... and at other places.
6. Move the hook_help at the top of the file http://cgit.drupalcode.org/sandbox-vineethaw-2478405/tree/responsive_sli...
7. Readme.txt is nice.

No Blocker comes up

Thanks for your contribution, vineethaw!

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.