[D7] Responsive Blocks

Git clone command for the sandbox is missing in the issue summary, please add it.

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.

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

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

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.

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

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/responsive_blocks.module
--------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
--------------------------------------------------------------------------
123 | ERROR | [x] Array indentation error, expected 4 spaces but found 6
124 | ERROR | [x] Array indentation error, expected 4 spaces but found 6
125 | ERROR | [x] Array indentation error, expected 4 spaces but found 6
183 | ERROR | [x] Array indentation error, expected 4 spaces but found 2
184 | ERROR | [x] Array indentation error, expected 4 spaces but found 2
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/responsive_blocks.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
22 | WARNING | Do not use drupal_add_js() in hook_init(), move it to your
| | page/form callback or use hook_page_build() instead
---------------------------------------------------------------------------

Manual Review

Use hook__preprocess_html(&$vars) instead of hook_init(). Using hook_page_build() as suggested from pareview.sh will result in one more warning.

No duplication
Yes: Does not cause module duplication and/or fragmentation.
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. / No: List of security issues identified.

Fixing priority, "Critical" is only for applications in "Needs Review". See https://www.drupal.org/node/539608#application-priorities.

Automated Review
The automated found some small errors, check them out at https://www.drupal.org/node/2316301#comment-9707687

Manual Review

Individual user account
Yes: Follows

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
No: Does not follow 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

README.txt
(+) You don't have any introduction, installation or configuration sections

Misc
1) I found that the git clone details in the issue summary were trying to connect via your account, prompting me to enter your password.
Type it like this instead:
git clone --branch 7.x-1.x git.drupal.org:sandbox/SwapS/2229089.git responsive_blocks

2) Also there is a file in your /js directory that is called 1 and is a copy of responsive_blocks.js except that it doesnt have a file extension. So you should delete this file since my guess is that it is not necessary.

Manual Review.

Instead of using drupal_add_js(), It is always recommended to use $form['#attached']['js'].

  drupal_add_js(array('responsive_blocks' => array('responsive_blocks' => $js_variables)), 'setting');
  drupal_add_js(array('responsive_blocks' => array('device_width' => $device_width)), 'setting');

Refrence:
http://drupal.stackexchange.com/questions/90040/what-is-the-difference-b...

Hi SwapS,
Thanks for contributing.

Manual Review

There are some issues I found:
responsive_blocks.module

1. line 11 - responsive_blocks_help() - missing return statement. To fix it you should return some string by default, e.g. empty string $output = '';
2. line 29 - absolutely unnecessary define arrays separately, they will be created in the first definition (minor).
3. line 121 - variable $response_widths overwrited by row below, so this row is useless and can be removed.
4. you are using variables (responsive_blocks_block_settings, responsive_blocks_desktop, responsive_blocks_ipad, responsive_blocks_mobile), but they won't be deleted after uninstallation of module. Needed to add corresponding code in .install file.

responsive_blocks.js

1. line 17, 19 - unused variables blocks and blocks_array

  $settings = variable_get('responsive_blocks_block_settings', array());
...
    '#default_value' => empty($settings[$module][$delta]) ? array() : $settings[$module][$delta],

You're putting the settings for ALL blocks into a single variable. That's going to be dreadful for performance on sites with a lot of blocks. You need a table for this.

function _responsive_blocks_get_library_path() {

This doesn't seem to get called at all.

  if (function_exists('libraries_get_path')) {

Consider simply making Libraries module a dependency.

function _responsive_blocks_get_readme() {
  $readme = file_get_contents(dirname(__FILE__) . '/README.txt');

Slurping in the README file for your hook_help() isn't very good practice. Generally, a README contains details such as what the module does from a 'why should you use this?' POV, and installation instructions. Neither of these are relevant in hook_help(). Also, hook_help() should give links to admin pages, which a README can't.

  $form['desktop'] = array(
    '#type' => 'textfield',
    '#title' => t('Desktop Resolution - Start point'),
    '#description' => t('Enter the Desktop Resolution Starting point in pixel'),
    '#default_value' => variable_get('responsive_blocks_desktop', 1600),
    '#size' => 40,
    '#required' => TRUE,
  );

You should really consider making use of existing contrib modules that let users define breakpoints. It's highly likely that users need to define breakpoints for other uses, and this makes them duplicate that information.

Additionally, there are some code formatting issues, especially around multi-line arrays.

Automated Review:

FILE: /var/www/drupal-7-pareview/pareview_temp/responsive_blocks.module
---------------------------------------------------------------------------
FOUND 6 ERRORS AND 2 WARNINGS AFFECTING 5 LINES
---------------------------------------------------------------------------
16 | WARNING | [ ] Empty return statement not required here
194 | ERROR | [x] Array indentation error, expected 6 spaces but found
| | 8
194 | ERROR | [x] Array closing indentation error, expected 6 spaces
| | but found 8
196 | ERROR | [x] Array closing indentation error, expected 4 spaces
| | but found 6
200 | ERROR | [x] Array indentation error, expected 6 spaces but found
| | 8
200 | ERROR | [x] Array closing indentation error, expected 6 spaces
| | but found 8
202 | WARNING | [x] A comma should follow the last multiline array item.
| | Found: )
202 | ERROR | [x] Array closing indentation error, expected 4 spaces
| | but found 6
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 7 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
------------------------------------------------------------------------
64 | WARNING | [ ] Line exceeds 80 characters; contains 100 characters
65 | WARNING | [ ] Line exceeds 80 characters; contains 89 characters
71 | ERROR | [x] Expected 1 newline at end of file; 3 found
------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------

Manual Review:

responsive_blocks.module

Line 16:

I think this return is extraneous and can be removed. Not a biggie.

Line 142:

Typo: Select options if block needs to be displayed on specific resolutions( Specific devices ) should be Select options if block needs to be displayed on specific resolutions (specific devices).

Review of the 7.x-1.x branch (commit 78f371b):

• ESLint has found some issues with your code (please check the JavaScript coding standards). See attachment.
• 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:

1. project page is too short, what is this module about? Please fill that out according to https://www.drupal.org/node/997024
2. Repeating "Issue #2316301: [D7] Responsive Blocks" as git commit message does not provide any useful information. The message should tell me what you did in that commit. See https://www.drupal.org/node/52287
3. What are the differences to https://www.drupal.org/project/context_mobile_detect and https://www.drupal.org/project/browscap_block and others? Please add that to the project page.
4. _responsive_blocks_get_readme(): since the README is not user provided text the check_plain() calls are not needed here.
5. responsive_blocks_form_block_admin_configure_alter(): 'Mobile' for example is user facing text and must run through t() for translation.
6. responsive_blocks_block_view_alter(): don't call drupal_add_js() here, use #attached for that.
7. responsive_blocks_admin_settings(): doc block is wrong, this is a form constructor. See https://www.drupal.org/coding-standards/docs#forms

Although you should definitely fix those issues they are not critical application blockers, otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

Oops, forgot attachment.

Review of the 7.x-1.x branch (commit 4e94e77):
Manual Review :

1. Project page is too short and not specifying for what the module is for ? See the tips for project page https://www.drupal.org/node/997024
2. responsive_blocks_admin_validate : you can also use element_validate_integer_postive for validating intetger numbers using #element_validate key in form element.It will do the validation for you in free.
3. _responsive_blocks_get_readme : This function is not called anywhere ?
4. responsive_blocks_preprocess_html : Its prefferable to use #attached here instead of drupal_add and its friends.Similarly in responsive_blocks_block_view_alter

Thanks for your contribution, SwapS!

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.