[D7] Multimedia block

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.

Automated Review

Pareview.sh - There is just one complain, http://pareview.sh/pareview/httpgitdrupalorgsandboxmyvo2509294git

Coder Review

multimedia_block.admin.inc

• Line 39: When labelling buttons, make it clear what the button does, "Submit" is too generic.
      '#value'      => t('Submit'),

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

Yes: Meets security requirements.

Coding style & Drupal API usage

List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. You should provide the hook_help to allow site builders to find information about your module using Drupal UI.
2. I think you should separate the responsiveslides.min.js from your project into sites/all/libraries, which is where we usually store our libraries. responsiveslides.min.js use the MIT license so this means that you can stored inside of your module according to this comment.
3. You should remove all your variables once the module is uninstall. Right now, if I uninstall this project all the variables from this project are going to still on the website.

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.

I do not see any blockers; however, you should consider fixing those things that I mention.

no, third party code is not allowed in drupal.org git repositories. The white list is only used for packaged downloads of Drupal distributions.

PAReview: 3rd party code

responsiveslides.min.js appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Hi @myvo thanks for contribution

i had tested this module on my local machine i found error when i was enter machine name.
Please link help in first image (Modules sumit.png) with readme.txt file

Hi myvo,

You should add below case in hook_help to allow site builders to find information about your module using Drupal UI.

case 'admin/help#multimedia_block':
$output = "information about your module";

In addition, you can also link help with readme.txt file by adding
$output .= file_get_contents(drupal_get_path('module', 'multimedia_block') . '/README.txt');

Thanks My,

This module is great.
It helps us to create simple and effective blocks.

Automated Review

No errors detected by automated review

Manual Review

Individual user account

Yes: Follows guidelines for individual user accounts.

No duplication

Yes: Does not cause 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

Yes: Meets the security requirements.

Coding style & Drupal API usage

List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. You use Libraries API but don't list it as a dependency. Also, JS library is placed in a root libraries folder. It should be inside its own folder to keep it organized. See this page for details.
2. In hook_uninstall you should not use LIKE % conditions, as it may accidentally uninstall variables of other modules. Better explicitly set what variables should be deleted.

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.

Hi myvo,
1. According to me, there is no specific need of $js_filepath variable in your multimedia_block.module. (line 623)
You can instead use

if(multimedia_block_get_library_js())
{
  drupal_add_js(multimedia_block_get_library_js());
}

2. You can preferably move this section to a separate function and call that function wherever needed.

$mpath = drupal_get_path('module', 'multimedia_block');
  drupal_add_css($mpath . '/misc/css/mblock.css');
  drupal_add_js($mpath . '/misc/js/mblock.js');

Point 2 is definitely not a mandatory one, but of course a better coding practice.

Please mention if I am wrong anywhere.

Hi myvo,
1. According to me, there is no specific need of $js_filepath variable in your multimedia_block.module. (line 623)
You can instead use

if(multimedia_block_get_library_js())
{
  drupal_add_js(multimedia_block_get_library_js());
}

2. You can preferably move this section to a separate function and call that function wherever needed.

$mpath = drupal_get_path('module', 'multimedia_block');
  drupal_add_css($mpath . '/misc/css/mblock.css');
  drupal_add_js($mpath . '/misc/js/mblock.js');

Point 2 is definitely not a mandatory one, but of course a better coding practice.

Please mention if I am wrong anywhere.

Hi myvo,

Firtstly, Great plugin! I think this plugin will help we create fast portable slideshow or media.
Secondly, when I create a 'Embed Video' block type and this block type has confuse me. Is it convenient when you put image is required ?
Thirdly, Slideshow block type is great. I think it is very usefull. But there are one problem. Can you fix the issue when user forget upload their image at the backend.? The slideshow on the front end still be smooth. Currently, If the user forget input their image the front end will miss that part.

Manual review
1)Displays error if there is no library Uncaught TypeError: $(...).responsiveSlides is not a function mblock.js
Better probably so

/**
 * Add the multimedia block css and js.
 */
function multimedia_block_add_css_js_files($jslib = FALSE) {
  if ($jslib) {
    $mpath = drupal_get_path('module', 'multimedia_block');
    // Add ResponsiveSlides.js lib to render slideshow.
    $js_filepath = multimedia_block_get_library_js();
    if ($js_filepath) {
      drupal_add_js($js_filepath);
      drupal_add_js($mpath . '/misc/js/mblock.js');
    }
  }
  drupal_add_css($mpath . '/misc/css/mblock.css');
}

2) Instead of public:// use

file_default_scheme();

to give the user a choice.
3) Use single quotes better quotes
4) Please add title and alt for images it is need for SEO
5) Please add a link https://github.com/viljamis/ResponsiveSlides.js/archive/master.zip to hook_help().
6) Need use block_load($module, $delta) instead

  $query = 'SELECT * FROM {block} WHERE module = :module AND delta = :delta';
  $block = db_query($query, array(':module' => 'multimedia_block', ':delta' => $block_delta))->fetchObject();

Look like you store all list of block to this variable, should be a problem if site has a lot of block?

$blocknames = variable_get(MULTIMEDIA_BLOCK_NAMES_VAR, array());

Block type is hardcoded, is it possible for other module to provide custom block type? Why don't you use ctools plugin API?

function multimedia_block_get_type($key = 'all') {
  $list = array(
    // Block delta prefix => block type name.
    'mbimage'      => t('Image'),
    'mbembedvideo' => t('Embed video'),
    'mbslideshow'  => t('Slideshow'),
  );

  return isset($list[$key]) ? $list[$key] : $list;
}

I think multimedia_block_admin_is_exist_blockname($name) should not return false, isn't it $name there, why can't check?

/**
 * Is block name exist.
 *
 * We can not check exist at here because we can not get full block delta name,
 * so we always return FALSE.
 */
function multimedia_block_admin_is_exist_blockname($name) {
  return FALSE;
}

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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