[D7] Block Custom Title

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2448653

Project 2: https://www.drupal.org/node/2422715

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

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.

Assigning to myself for next review that may be tonight.

Individual user account

Yes

No duplication

Yes

Master Branch 

Yes

Licensing 

Yes

3rd party assets/code
Yes

README.txt/README.md 

No, not completely. While I understood it, it could be more complete. For example, you say a 'privileged user' can edit the blocks title. Looking at the code, only user 1, the administrator can. The text implies you could let a user use the module through permissions, when you can't. Also, you misspelled privilege.

Code long/complex enough for review

Yes

Secure code

Yes

Coding style & Drupal API usage
The function custom_blocks_title_create() does not have a params statement in the docblock. See https://www.drupal.org/coding-standards/docs#functions.

Passed Coder with flying colors.

Please find my review below:

Automated Review

No issues found on http://pareview.sh/

Manual Review

Individual user account

Yes: Follows.

No duplication

Yes: Does not cause.

Master Branch

Yes: Follows.

Licensing

Yes: Follows.

3rd party assets/code

Yes: Follows.

README.txt/README.md

Yes: Follows.

Code long/complex enough for review

Yes: Follows.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

custom_block_title.install

1. (*)You don't need to call drupal_install_schema() in custom_blocks_title_install(), schema definitions are automatically called by drupal even before calling hook_install() and automatically removed when uninstalled. Also the passed in name in drupal_install_schema() is incorrect, you need to pass module name in it.

custom_block_title.module

1. (+)current_path() can be used instead of implode("/", arg()) to reduce extra processing.
2. $destination variable is duplicate or is of no use, use $path_url for setting both (path and destination) parameters.
3. (+)Instead of using db_query in custom_blocks_title_create use db_merge() as it will reduce the overhead of processing and checking.
4. After entering title and then again removing it, there is an garbage record left in your module table. I suggest that you delete the entry if a user submits a blank title to maintain clean records.
5. Instead of looping through the records returned by db_query(), use Database API functions like execute() and fetchAssoc() to get a single record from your module table in custom_blocks_title_form_alter() and custom_blocks_title_block_view_alter() functions.
6. You can store arg() function values in an array instead of calling it more than once in custom_blocks_title_form_alter() function.
7. Just suggestion, use inline comments to improve code readility.

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.

$_GET['path'] is not directly concatenated into a query string, so there cannot be SQL injection? drupal_get_query_parameters() will not sanitize $_GET either, it should only be used to remove unwanted elements (as the documentation says https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_ge... ). It is only used in #value here, which is fine.

Make sure to test the actual vulnerability next time :-)

My mistake, it will handle that in db_query. will surely take care next time. thanks for correcting. :)

Individual user account
Yes: Follows

No duplication
Yes: Does not cause

Master Branch
Yes: Follows.

Licensing
Yes: Follows

3rd party assets/code
Yes: Follows

README.txt/README.md
Yes: Follows

Code long/complex enough for review
Yes: Follows

Secure code
Yes: Meets the security requirements.

Coding style & Drupal API usage

custom_blocks_title.module
You have a variable on line 87 that is not being used. As it serves no function, just remove it. :)

@jzasnake,
Above is not blocker.Is there anything else that stopped you to set this to RTBC :)

@naveenvalecha
Sorry this is my first review, should I set it as RTBC? Because that was the only thing that i found.

No Need to say sorry.Yup if you have not found any blocker you can set it RTBC :)

Great, thank you for the help! :)

Manual Review (Read 192c82c...)

1. (*) Project page needs updation.See the tips for better project page https://www.drupal.org/node/997024
2. your git commits are not connected to your user account. You need to specify an email address. See https://www.drupal.org/node/1022156 andhttps://www.drupal.org/node/1051722
3. (*)custom_blocks_title_preprocess_block : Use theme_image instead of preparing the image html here.
4. (*)Need to delete the data when the block will delete.hook_form_formid_alter will fits the right here.Something like custom_blocks_title_form_block_custom_block_delete_alter(&$form, &$form_state){.........}
5. The module has name custom_blocks_title I would suggest it should be something like block_custom_title because it looks better to me.Its a suggestion.
6. custom_blocks_title_form_alter : Implement hook_form_form_id_alter that will gives some performance benifits as well and will only hits that particular form.
7. custom_blocks_title_form_alter : It would be better to use drupal_get_query_parameters over $_GET
8. hook_help() is missing in this module.It would be nice to add it.
9. (*) If I'll directly access this path block-title/%/%/editblock-title/system/navigation/edit Then I am getting these errors
   

   Error message

   • Notice: Undefined index: path in custom_blocks_title_form_alter() (line 101 of /Applications/MAMP/htdocs/d7.dev/sites/all/modules/sandbox/custom_block_title/custom_blocks_title.module).
   • Notice: Undefined variable: custom_page in custom_blocks_title_form_alter() (line 108 of /Applications/MAMP/htdocs/d7.dev/sites/all/modules/sandbox/custom_block_title/custom_blocks_title.module).
   • Notice: Undefined variable: custom_page in custom_blocks_title_form_alter() (line 111 of /Applications/MAMP/htdocs/d7.dev/sites/all/modules/sandbox/custom_block_title/custom_blocks_title.module).

   Need some vaildation here.

Removing Review bonus.Please take another Review bonus for the second admin review.

Looks like you forgot the review bonus tag?

manual review:

1. block_custom_title_preprocess_block(): looks like this should be done with contextual links instead? See https://www.drupal.org/documentation/modules/contextual and https://www.drupal.org/node/1089922
2. block_custom_title_create(): why do you get $custom_page from $form['define_page']['#value'] and not $form_state['values']? Please add a comment.
3. block_custom_title_form_alter(): doc block is wrong, this is hook_form_alter().
4. block_custom_title_form_alter(): why are you altering your own form? You can set the #default_value in block_custom_title_form() and also change #access there? And the elements should be #type => 'value' if they are not displayed anyway. See https://www.drupal.org/node/36050
5. block_custom_title_form(): this function could receive the 2 additional parameters from the URL. You should specify "'page arguments' => array('block_custom_title_form', 1, 2)," in hook_menu(), see https://api.drupal.org/api/drupal/modules!system!system.api.php/function... . Then you don't need the crude arg() hack in block_custom_title_form_alter().
6. block_custom_title_block_list_alter(): why is this hook needed? Please add a comment.

But otherwise looks RTBC to me.

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

no objections for more than a week, so I'm taking a final look.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. None

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

Git errors:

• The last commit message is just one word, you should provide a meaningful short summary what you changed. See https://www.drupal.org/node/52287

• 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

.info: description = Block customization functionality not a useful info. It should be something like "Allow block title to be changed for different pages."

Looking at the git history at https://www.drupal.org/node/2447165/commits, one or two word git commit message like "Modifications" many times does not really help your git history. See https://www.drupal.org/node/52287 on how to write meaningful messages.

block_custom_title_block_list_alter(): arg() is evil, and should almost always be avoided.

But otherwise looks good to me.

Thanks for your contribution, Sajini antony!

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.