[D7] Block Manager

• Fix issues that PAreview finds http://ventral.org/pareview/httpgitdrupalorgsandboxgrandivory2008620git
• Images folder contains remove.psd, I guess that is redundant.
• Your jQuery could be better, http://api.jquery.com/jQuery.noConflict. Drupal uses jQuerys noconflict, but that does'nt mean you can't use $ variables. Maybe this helps: http://browse-tutorials.com/tutorial/using-jquery-drupal-7
• I don't think you need separate module package. If all modules had packages with their own name, then I don't see the point.
• Remove master branch: https://drupal.org/empty-git-master

When I open the block configuration manager I don't get the limit_types checkbox in the form. It causes the error below. Checking the code it looks like that is because I do not have nodeblock installed. You should either make your module dependent on nodeblock, or fix the submit function so that it's not looking for the field when it doesn't exist.

Notice: Undefined index: limit_types in block_manager_admin_settings_validate() (line 109 of /home/quickstart/websites/test.dev/sites/all/modules/block_manager/block_manager.admin.inc).

PDOException: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'limit_types' at row 1: INSERT INTO {block_manager_config} (theme, region, manage, layout, limit_types, allowed_types) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => bartik [:db_insert_placeholder_1] => header [:db_insert_placeholder_2] => 1 [:db_insert_placeholder_3] => horizontal [:db_insert_placeholder_4] => [:db_insert_placeholder_5] => s:0:""; ) in block_manager_admin_settings_submit() (line 183 of /home/quickstart/websites/test.dev/sites/all/modules/block_manager/block_manager.admin.inc)

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.

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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. block_manager_page_alter(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075 . Also elsewhere.
2. "'limit_types' => !!$c->limit_types,": why the double exclamation mark? Please add a comment.
3. _block_manager_get_region_blocks(): "db_select('node', 'n')": looks like this query does not respect node access grants? This could be an access bypass security issue, so this is a blocker. See http://api.drupal.org/api/drupal/modules!node!node.module/group/node_acc... . And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
4. _block_manager_get_region_blocks(): you are querying all nodes of the site here if nodeblock is not enabled? This will fail horribly on a site with one million nodes.
5. block_manager_node_delete(): why are you deleting stuff on behalf of nodeblock module? The module should do that cleanup itself?
6. block_manager_admin_settings(): do not document $form and $form_state, see https://drupal.org/node/1354#forms
7. "'#markup' => '<p>Select the regions that should be managed by the block manager instead of the default block module</p>',": all user facing text must run through t() for translation. Same for #options like "'vertical' => 'vertical',".
8. block_manager_remove_blocks_form(): This is vulnerable to XSS exploits. $page is user provided input and needs to be sanitized before printing in confirm_form(). Use t() with the appropriate placeholders here to escape it. Make sure to read http://drupal.org/node/28984 again.
9. block_manager_remove_blocks_form_submit(): this is vulnerable to open redirect exploits. The $pages[0] variable is directly derived from a user provided value, right? You need to make sure the destination is local to your site. Setting a destination GET parameter should be enough for your use cases anyway, right? See drupal_goto() drupal_get_destination() and friends.

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

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

manual review:

1. _block_manager_get_managed_regions(): you don't need the foreach loop, just use ->fetchAllKeyed() or ->fetchAllAssoc() or a similar function.
2. If you want to force a boolean then you should use a cast (bool), not "!!".
3. block_manager_remove_blocks_form(): The ":" placeholder does not exist for t(), I think you want to use "%" to get the em tags. Please read the documentation of t() again.
4. "'#title' => 'Remove all blocks',": all user facing text must run through t() for translation. Please check all your strings.
5. "t('Create new') . ' ' . $info->name,": do not concatenate translatable strings like that, use placeholders with t() instead.
6. block_manager_admin_settings(): there is a very long conditional code line that is absolutely unreadable, please split that up into proper if statements. Control structure conditions should also NOT attempt to win the Most Compact Condition In Least Lines Of Code Award™. See https://drupal.org/coding-standards#linelength

Otherwise looks almost ready, but knowledge about t() placeholders is an application blocker.

manual review:

1. _block_manager_copy_block_db(): I think you can just foreach over $page_blocks instead of the while loop.

But 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 kscheirer as he might have time to take a final look at this.

Big module, but it looks good to me. No problems found, and the sql injection reports from ventral look spurious. I can see this being an easier alternative to Context, which can be difficult to explain to clients.

Thanks for your contribution, grandivory!

I updated your account to let you 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 get 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.

----
Top Shelf Modules - Enterprise modules from the community for the community.