[D7] Background Video

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

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

Hi Pankaj,

Its really nice module. Some points I have to mention :

1. Why to initialise the variables in javascript file? Just use settings.background_video.ogv directly instead of setting the variable first.
2. Instead of Drupal.settings, you can directly use settings (attach: function (context, settings)).
3. Your comments says wrong for background_video_preprocess_page function.
4. You have added hook_library in your .module file. It should be used instead of adding drupal_add_js in your hook_preprocess_page. Please use 'background_video' instead of 'background_video_js' and add css file as well.
5. Please use a default value in every variable_get you use otherwise it will throw an error till the variable is not set.
6. I can see there is no check if video or some configuration not available and your javascript file will be loaded on every page in any case.

Automated Review

The pareview.sh report for the project looks good. No automated test cases were found, but that is no requirement.

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.

Please add a section about similar projects to your project page. It's fine, if you provide a short, high-level overview of the differences.

Once approved, your project will probably be available at https://www.drupal.org/project/background_video, not https://www.drupal.org/background as mentioned in the REAMDE file.

README and project page might be improved further by more closely following the README template (linked above) and the project page template, but the basic information needed is there. You also might want to check out the tips for a great project page.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No: Does not meet the security requirements. See below for information on the issue(s) identified.

Coding style & Drupal API usage

1. (*) XSS: You're working with the raw values entered by the user without filtering them for XSS in some cases. This is a security issue! You need to sanitize all insecure values using either check_plain(), filter_xss() or filter_xss_admin() before output. To test, try the following:
   • XSS for ID/Class name setting:
     1. On your module's settings page, fill in all required settins. For the ID/Class name setting, use <IMG SRC=/ onerror="alert(String.fromCharCode(88,83,83))"></img>.
     2. Once you save, you should see a popup displaying XSS.
2. (*) background_video_menu(): Why are you using administer users permission for the settings page? Please implement hook_permission() to provide your own permission and check for that in hook_menu().
3. (*) background_video_uninstall(): Please use variable_del() to delete your variables.
4. (+) background_video_form(): This is not an implementation of hook_form(), but a form builder function for your module settings form. Please read the documentation on hook_form() for information on what hook_form() does. Consider renaming the function to a different name to avoid confusion (e.g. background_video_admin_form). At least, please update your DocBlock.
5. (+) background_video_form() at line 21: Please use a token for the expected library path so that the string doesn't need to be retranslated, if the path ever changes.
6. (+) background_video_form(): Your files are saved to file_managed table as temporary files. Since no module claims usage, the files will be deleted by system_cron() once they are older than DRUPAL_MAXIMUM_TEMP_FILE_AGE (see file unmanaged save upload and How is the file data in the database table file_usage created and managed?).
7. (+) background_video_preprocess_page(): The DocBlock should read Implements hook_preprocess_HOOK() for page.tpl.php..
8. As a best practice, please use single quotes instead of double quotes unless in a few very specific cases (see Drupal Coding Standards). There are several places in your module, where you use double quotes instead of single quotes.
9. background_video_form(): Why do you check, if the function libraries_detect exists? Since you added the libaries module as a dependency, the function should always be available.
10. background_video_form(): Consider to use the configured default scheme for the upload location of your video files ($scheme = variable_get('file_default_scheme', 'public') . '://'; should get you going).
11. background_video_form(): Note, that form elements of type checkbox do not use the #options property (see Form API reference).
12. background_video_preprocess_page(): You're using different default values for your variables here than in background_video_form(). As a best practice, use the same default values throughout the module for clarity.
13. Consider documenting the expected folder name for the library on your project page and in the README file to ease installation.
14. See README.txt/README.md section above for some advice on documentation for your project.
15. Consider to provide a setting to specify the path, where the video should be included. E.g. users probably don't want to include it on admin pages. You could then use drupal_match_path() in background_video_preprocess_page(). Or check, if path_is_admin() returns FALSE.
16. Consider to allow png as an extension for poster images, if the library you use allows for 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.

Revision reviewed: 634a597

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.

Hi Pankaj,
Nice module.

Manual Review:

1. It is important to have a custom permission. And not use the
'access arguments' => array('administer users'),
If we gave a user role "User Manager" which are able to manage users only. The config will show up for the user managers too.

2. You do have a very restrictive validation for videos ( Why it is a must to have [ .mp4, .webm, .ogv ] at the same time?
We could have the validation to have one of them.

3. Uninstall actions Delete variables when. I think using variable_del will do better if the list of variables are less than 10.

Rewarded Module you have ;)

Great module, if I only had seen it before!

manual review:

1. In the function background_video_menu, make sure to use the t() function for the title and description attributes.

2. I saw that @sumitmadan told you to add a default value, but the default value for variable_get is already NULL, so you don't need variable_get('whatever', NULL). But you have to check if the value is null before you do something with it. For the boolean values (loop and autoplay), I would add a FALSE or a TRUE value in background_video_preprocess_page as you do in the form.

3. wrap css with the class of the module to avoid conflict with classes already present in the theme

4. As @RajabNatshah said, you shouldn't need to upload the three videos in order to work, can you instead put only one field that accepts three types of file?

Your code is well written and I was able to find only minor issues, but I would strongly recommend you to implement item 4.
Thanks for your module.

1. In the function background_video_menu, make sure to use the t() function for the title and description attributes.

Please don't run title or description in hook_menu() through t(). See Strings at well-known places: built-in menus, permissions, log messages and .info files for more information.

I am puzzled. Maybe I am misunderstanding something in the way Drupal handles variables.

In your code, there is not a single variable_set() call. Yet, you have several variable_get() calls to retrieve module specific variables using the name pattern background_video_*.

So basically, all these calls are always going to simply return the default value. It seems to me that some refactoring is needed there.

Finally, in hook_uninstall, you are calling variable_del() to delete several variables that are never set. More refactoring is needed here also.

The source code of variable_get is shown below. As you can see, it never sets anything either.

function variable_get($name, $default = NULL) {
  global $conf;

  return isset($conf[$name]) ? $conf[$name] : $default;
}

Hey pankajsachdeva,

Here is some errors, please look into it, nice module by the way.

http://pareview.sh/pareview/httpsgitdrupalorgsandboxpankajsachdeva271436...

Thanks,

Coding standards issues are not application blocker. Is there something else that is blocking the review or marking this a RTBC?

Hello pankajsachdeva.

My findings for your module as follows:

File: background_video\background_video.admin.inc
Line 93 : Missing function doc comment

File: background_video\README.txt
Line 25 :Line exceeds 80 characters; contains 115 characters

Thank you,

Hi pankajsachdeva,

Your module is looks nice, got below warning while uploading the videos in background video configuration page.
Warning : array_flip(): Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load()
you should look into the issue.

Hi @Yogesh Pawar!

Thank you for your reviews! Please make sure to change the status of the project after the review if applicable.
If the issue you found is a application blocker (I think it is), please change the status to "Needs work". If you think there is no blocker, then changing the status to "Reviewed and tested by community" will help application go ahead.

Automated Review

Here some error please fixed it
http://pareview.sh/pareview/httpsgitdrupalorgsandboxpankajsachdeva271436...

Hi,

$ogv_uri = $ogv_file->uri;
$url_ogv = file_create_url($ogv_uri);

$ogv_uri unnecessary variable.
Use $url_ogv = file_create_url($ogv_file->uri);

Coding style & Drupal API usage
1. The function background_video_preprocess_page() contains three similar blocks for retrieve url for ogv, mp4, webm.

You can create a helper function.

function _background_video_geturl_preprocess_page($type) {
  $fid = variable_get("background_video_source_$type", NULL);
  if (!empty($fid)) {
    $file = file_load($fid);
    if ($file) {
      return file_create_url($file->uri);
    }
    else {
      // Some error message.
    }
  }
  return NULL;
}

And then use this function in background_video_preprocess_page() :

$ogv_fid = _background_video_geturl_preprocess_page('ogv');
$mp4_fid = _background_video_geturl_preprocess_page('mp4');
$webm_fid = _background_video_geturl_preprocess_page('webm');

Hi pankajsachdeva,

I did a manual review. See my comments below:

1. Video is playing for all the admin pages also. Please include settings to play video for particular themes or add settings to include/exclude certain pages.
2. Add hook_requirements to check availability of jquery-videobackground library. Then it will add message in status report regarding availability of library.

Otherwise module working fine and looks good to me. Please fix above issues.

Thanks,
ARUN AK

1. As you are dsiplaying jquery-videobackground library status as error message, if we are uploading video video without library then repeated messages displaying in config page. See screenshot. I strongly recommended use of hook_requirements() to implement third-party libraries in drupal.
2. Didn't see any replay on some comments, #25, #30. Still that warning is coming after save the configuration page with uploaded video.
   

   Warning: array_flip(): Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load() (line 175 of C:\wamp\www\www_d7test1_com\includes\entity.inc).

   Please fix this issue.

Thanks,
ARUN AK

Hi pankajsachdeva,

It seems like have fixed the issues mentioned in my previous comment. I have done one more round of review and see my comments below:

As you are not processing any theme variables, use hook_page_build() instead of background_video_preprocess_page(&$variables) to add elements in to the pages.

Also noticed some alignment and z-index issues on video controls. In configuration I setup .region-sidebar-first as Control Position. But it is not displaying properly in that region. Please see attached screenshot.

It would be better to have Enable/Disable checkbox in configuration page to disable video temporarily. Currently either need to disable the module or need to remove videos from settings to disable background video.

Otherwise looks good to me. Moving to RTBC.

Thanks,
ARUN AK

Some fantastic reviews here! Especially from FeyP.
It sure looks like we should push this forwards.

Giving it a run locally, and seeing if it works OK...

* Enabled the module
* It required jquery_update (fair enough) - but it would have made sense to have listed that on the project page somewhere.
* code review on background_video.install:background_video_requirements() shows some major whitespace indent problems. I'm not gonna block on that (Everything else is well clean). But it could be fixed.
* The Status report shows the hook_requirements well, so that is well in line with Drupal expectations.
* Suggestions: a drush *.make file that can pull in the named 3rd-party library makes this module possible to automate tests for (simplytest.me and things)

I am freaked that the library we need to pull in is what? 16 M big !?
That's bigger than many full sites. How is that even possible?

* The manual install instructions were fine, and installing the lib went as expected.

* I was puzzled about the gap between installing the module and actually finding the setting or instructions for it. The project page, and the README, provided installation instructions but no actual usage instructions at all. This lack of user support makes it difficult to evaluate or test.

...
..
I seriously have no idea where to look next after installing this module. How do we make it work? But it seems that the previous reviewers were able to guess..
From the module page, I see a good link to /admin/config/media/background-video-settings ... That could be repeated on the usage instructions on the project page or README I think..

..
Looking at the config form. (Would be GREAT if your project page had a snapshot of the config form, that helps us evaluate what sort of control the module provides)

Sooooooooo?
This module sets only one video for your whole site ever?
No context, no tag-or-content control?

And, uh, I must supply 3 formats of the same video file before anything can be tested!? That's a true pain.
It actually took me many many minutes to find even a default sample ogv video of anything to use as a placeholder so that I could even submit the form successfully. This is a bit weird, given the solid "media" management support that has been made available to Drupal in the recent years.

It did actually place a video on my page when I got through that though. So it does at least work, when the UI problems have been fought.

* The background video does not "fill the page" as we expect from this sort of effect. it was scaling itself down to fit the space when the window is resized, instead of cropping and expanding. The config settings did not provide a clue as to how we could adjust that behaviour.

* Although the UI promised to provide video controls in the 'footer' of the page, these were not available.

* Without any control over which paths or pages the video was shown on, or the ability to have different videos in different contexts, this module seems *very* limited in utility, and not suitable for a CMS at all. It's really just a short step up from an HTML template.
In Drupal, we would expect some way to manage this display that was in some way related to the content being shown. Modules such as
- https://www.drupal.org/project/bg_image
- https://www.drupal.org/project/bg_image_formatter
- https://www.drupal.org/project/background_image_formatter
- https://www.drupal.org/project/dynamic_background
- And I am sure many more
... each provide a good amount of control over what is set as a background, and when. That would be the Drupally approach.

Looking at the code:

It's much too light on real comments or documentation.

/**
 * Implements hook_preprocess_HOOK().
 */

does not in itself provide any information about what you are doing, or, more importantly why.
Although the code is well-structured, it's still not what anyone could call self-explanatory enough to think you can leave it entirely undocumented.

Your comment:

/**
 * @file
 * This file provides basic functionality.
 */

Is quite simply, bullshit.

-----

It's quite weird that you found it necessary to filter for xss on a filename here

    '#default_value' => filter_xss(variable_get('background_video_source_mp4', NULL)),

but not in the next stanza here

    '#default_value' => variable_get('background_video_source_webm', NULL),

Can you explain why you found that mp4 filenames are more dangerous than webm filenames?

-----

The way that you have fixed on the 4 'formats' and treat them (and only themr) as all required, all the time is reflected in the code - via copy & paste repetition in the way these fields are handled.
This architecture will not scale, and is a bit short-sighted.

Suggestion: You could have a list of acceptable video formats (a pluggable definition list that contains their respective descriptions, capabilities, suffix etc), and put loops around your form UI to reduce the copy & paste code bloat.

----

Javascript and CSS is well done and very clean. Still somewhat lacking in maintainable documentation though.

----

Thanks to the reviewers above, the uninstall hooks are done well. Good job there!
(Whitespace errors (TABS!!?!) there are annoying, but are trivial to fix)

----

Suggestion:
(For next time, not worth a rewrite)

You can namespace your variables much quicker, and build a settings_form much quicker, if you use FAPI to set your form values to an array with '#tree'=TRUE, like

  variable_set('background_video', array(
    'video_source_mp4' => 'whatever',
    'video_source_ogv' => 'whatever',
    'video_source_webm' => 'whatever',
    'video' => array(
      'id' => 'whatever',
      'control_position' => 'whatever',
      'source_poster' => 'whatever',
      'loop' => 'whatever',
      'autoplay' => 'whatever',
    ),
  ));

Because that means that :

* retrieving your 'background_video' settings when you need them each time every page is just one DB lookup instead of 10
* cleanup later is much easier than the hard-to-maintain-manually:

  variable_del('background_video');
  variable_del('background_video_source_mp4');
  variable_del('background_video_source_ogv');
  variable_del('background_video_source_webm');
  variable_del('background_video_id');
  variable_del('background_video_control_position');
  variable_del('background_video_source_poster');
  variable_del('background_video_loop');
  variable_del('background_video_autoplay');

----

DESPITE all these grumbles, The use of the Drupal API is correct throughout, the module works correctly (within its limitations), it's as secure as can be expected, and provides a utility I've not seen elsewhere in the Drupal module list. If this effect is what you want, this code is a good enough way to get it.
There are a number of things that could be done better in order to become more useful - but if your site needs the one video coming up all the time behind every page... this module does that for you.

I endorse the RTBC, and think we can promote the account based on this code.

Yeah, this has been RTBC with no problems for too long. Definitely can be promoted.

Thanks for your contribution, pankajsachdeva!

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.