[D7] search_replace_blocks_menus

My review of the module:

1. pareview.sh reports no problems (test result).
2. README.txt file has a lot of unnecessary whitespace in the beginning of the file. It also lacks expanded description of what the module does. It states that "This module Search and replace a string in all custom added blocks and menus." but in regard to blocks it only searches in custom block body (neither block title nor block description is searched).
3. Path to module configuration is "/admin/config/system/replace-blocks-menus" but it can only be opened if user manually enters this path in the browser address bar as no administrative menu link is created (menu link type in search_replace_blocks_menus_menu() is MENU_LOCAL_TASK rather than MENU_NORMAL_ITEM). Description attribute for the menu link is also missing.
4. Module form presents who checkboxes that user can select ("Search Or Replace In Blocks Body." and "Search Or Replace In Blocks Body."). If form is submitted with no checkboxes checked, an error is displayed: "Please Select Atleast One Checkbox!". My suggestion is to use form element with type #checkboxes with #title attribute "Search and replace in following locations" with two options "Custom block body" and "Menu title" and #required attribute set to TRUE so that user knows that at least one of checkboxes needs to be checked.
5. If suggestion in previous point is taken into account, an error message (that has a space missing with almost all the words capitalised, btw) in search_replace_blocks_menus_input_form_validate() will not be necessary.
6. Once form is submitted, form values are stored in $_SESSION. Upon succeeding form opening, the values from $_SESSION are populated in input fields. I find it redundant. Even if there is a valid use case for this approach, the reset button should be provided that resets the whole form.
7. Module form submit handler functions are named search_replace_blocks_menus_form_submit_one and search_replace_blocks_menus_form_submit_two and their function comments state Submission handler for node_generator_input_form form. which is not descriptive enough of what the functions do.
8. search_replace_blocks_menus_form_submit_one and search_replace_blocks_menus_form_submit_two functions invoke drupal_set_message() with unsanitised output which has security implications.
9. search_replace_blocks_menus_form_submit_two function perform submitted value validation (if ($form_state['values']['replace_string'] == '') { .. }) which should be done in validation handler function.
10. Permission description in search_replace_blocks_menus_permission() should be capitalised.
11. An error "Notice: Undefined variable: key in search_replace_blocks_menus_form_submit_one() (line 95 of /Sites/search_menus/drupal/sites/all/modules/search_replace_blocks_menus/search_replace_blocks_menus.admin.inc)." is thrown if user searches for certain text in block body but no matches are found.

ankitgarg,

1. Regarding unsanitised output. Try checking "Search Or Replace In Menus Title" and enter <script>alert('Boom!');</script> in Search field. A javascript will be executed in your browser.

2. Upon form submission checkboxes are not preserving their state, that is, their "check" state is not populated.

3. drupal_set_title(t('Replace In Blocks And Menus.')) in search_replace_blocks_menus_input_form() form function is redundant as page title is already set by the menu system.

Multiple Applications

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

Project 1: https://drupal.org/node/2267323

Project 2: https://drupal.org/node/2262881

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.

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.

Module is working for me. Great work

The XSS concern in the first comment has not been addressed.

If I use the replacement text

<script>alert('XSS')</script>

I get alerts, I suspect from the manual output build to the drupal_set_message(). See https://www.drupal.org/writing-secure-code I only tested with block body, please also test the menu title handler.

Once you take care of that I will do a full review.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Nice module.
But please make these small changes:

• Please change @file comment from node_generator to your current module name in search_replace_blocks_menus.module file.
• You need to do some commenting in you module and admin.inc file to explain what action you are performing. Like in line 89, 104, 142 and 162 in search_replace_blocks_menus.admin.inc
• Search is spelled wrong in line 69 of search_replace_blocks_menus.admin.inc file.
• And pareview is also showing extra spacing issues in 5 and 69 of search_replace_blocks_menus.admin.inc

Removing review bonus tag, you can add it again after reviewing another 3 projects.

You have a lot of string literals not wrapped in t().

For example, in search_replace_blocks_menus.admin.inc on line 98:

$output .= '<span class="msg' . $key . '"><a href="' . $base_url . '/admin/structure/block/manage/block/' . $value->bid . '/configure">Search String Found in Block "' . $value->info . '".</a></span><br />';
      

Would probably be better as this:

$output .= '<span class="msg' . $key . '">';
$output .= l(t('Search String Found in Block @block_name', array('@block_name' => $value->info)), 'admin/structure/block/manage/block/' . $value->bid . '/configure');
$output .= '</span><br/>';

It could also afford to have more detailed inline comments.

Don't forget to set the status when you are done a review. See https://www.drupal.org/node/532400 for more info.

Also, it does help to use the review template, https://groups.drupal.org/node/427683, to make sure you cover all of the necessary points needed by a review.

I have not done a full review of this (I stopped a while ago when I found the XSS issues), widespread lack of t() and widespread non-use of API functions is a valid reason to go back to Needs Work.

Hi
i test this module manually and found some bugs/errors.
there is no methode for handling with HTML or special characters.

see in image frist link is replace by your module.
before its = replace and replace with
replace
and result is <h1>replace<h1>
and see the second one

its add by manually and its showing tags as it is. its mean your module behavior and drupal default behavior is not same.
second one if you strip all HTML tags and i am using Menu HTML then whats happened.
then you get this issue: your module is not compatible with Menu THML.
2 . you are using like query.
'%' . db_like($tmp) . '%', 'LIKE'
what should be happened.
i have menu

another link
link
unlink

now replace link with unlink
i want to replace only the second one but result is
another link become => another unlink
link become => unlink
unlink become => ununlink

also its effect my other menus also.

same in block body replace.

i have 10 block and need changes in only one block its not posible with this module.
i think you need alot of work with these issues

Automated Review

No issues found.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes.

Coding style & Drupal API usage

1. (*) I do not see any point of storing values inside $_SESSION. If you need these values to be inside the form after its submission, you can simply use $form_state['rebuild'] marker, instead of using session scope.
2. (*) Using form element type 'checkboxes' and more over getting values from those checkboxes after the form submission is inappropriate. First of all, you need to use 'checkbox' form type element instead of 'checkboxes'. Secondly, use $form_state['values'] to get form submission results
3. (*) Setting default values inside form is also not good. We can do smarter than adding 5 lines (meaning conditional statement if () {} else {}) to set every default value (totally 25 lines of useless code). For this we have array operator + (see http://php.net/manual/en/language.operators.array.php) which makes things short and nice
4. To show what I mean exactly in 3 points above, apply git patch I've attached for you. As extra suggestion, I would rebuild 2 submission handlers into 1, as they have many common things and same code.

Hi Ankit,
I don't know if this module is useful or not. But I like the UI you provided.

There are some points :

1. README.txt does not follow the drupal standard.
2. Currently search and replace are case sensitive, if I replace 'test' with 'test 2' then Test is not got replaced. There should be an option so it replace all the text without checking case sensitivity.

Removing automated reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

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

• 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: what is the use cases of this module, why would I need it? Please add that to the project page, I don't understand why I would want to replace all strings in custom blocks? See also https://www.drupal.org/node/997024
2. "$oprator" should be $operator, right?
3. $tmp is not a useful variable name, I immediately forget what it is about. Please use something meaningful like $filter or similar.
4. "preg_replace('/\b' . $scanner['search'] . '\b/', check_plain($scanner['replace']), $value->body);": do not use check_plain() here since you are not printing something to HTML here. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from https://www.drupal.org/node/28984

So the wrong check_plain() usage is a blocker right now. The review bonus tag is already removed, you can add it again if you have done another 3 reviews of other projects.

Hello,

• When I search and replace a string for a block value and Case insensitive checked the whole body is turned to Uppercase. Is there a reason for doing that (.module :line:154). I mean it could make sense, if you search only a string's insensitive value and replace only the given string .
• You set a $form_state values in your form array in order to provide the default values. There is no need to do that. You can Check the Form API on how to use the default value for your checkboxes.
• You can use a validate function in order to check if the user entered the values you expect. For example you can make sure user checked at least the blocks location.

Not adding anything myself but setting back to Needs Work after @Christina Kal's review.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

I tested the module:

I made a custom block called "custom test" with content "My first test". I saved it in first sidebar.

Then in admin/config/system/replace-blocks-menus I searched "first":

Result:

Search String Found in Block nur test
No Match Found For Search String In Menus.

Then I tried to replace with "second".
Result:

Notice: Undefined variable: body in search_replace_blocks_menus_form_submit_two() (Zeile 161 von /var/lib/openshift/55242d245973ca57fb00001c/app-root/data/sites/all/modules/search_replace_blocks_menus/search_replace_blocks_menus.admin.inc).

The block is gone and in block administration I can see that the body content is removed.

I assume this is not what you want to???

Then I went through your code from line 149 - 158:
In fact you write that at least one checkbox has to activated! So I would write this in your from description and make an activated checkbox as default, or you take radio buttons or a multi select from field. Doesn't matter.

Here's the code to make your module more secure!

Hi

Great job.!!
I downloaded and used your module.
It works fine for me.
Only, I have to clear the cache to reflect the changes.
Also, it would be great if you document your project properly.
Please visit "https://www.drupal.org/node/2190239".
It would be great if provide the description of what your module does a bit more ea
And I have seen some new issues logged by "http://pareview.sh/pareview/httpgitdrupalorgsandboxankitgarg2262793git".
So, please fix these and good luck :).

README.txt

The module description could be improved by changing it from:

This module Search and replace a string in all custom added block body and menus title.

to something more clear like:

This module will search and replace text on custom block bodies and all menu titles.

Also it was not clear to me from the description in README.txt if the module would search only custom menu titles or not. As it turns out, it searches all menu titles.

Not a blocker, but you might want to change this if you want people to use your module.

Coding style & Drupal API usage

1. + I got the same result as maen in #35. I created a new block, searched for text in its body, then attempted to perform a replace. The result was an error:

   Notice: Undefined variable: body in search_replace_blocks_menus_form_submit_two() (line 161 of ../sites/all/modules/contrib/search_replace_blocks_menus/search_replace_blocks_menus.admin.inc).

   And the block was changed so that I had to resave it for it to behave normally.

2. * The form would be improved by adding some descriptive text at the top and #description text to the checkboxes. It isn't clear what is going to happen when I click the Search button and it REALLY isn't clear what a disaster it could be when I click the Replace button.

   At the very least, add a bit of warning above the Replace button that will give users an idea of what kind of damage they could potentially do. The replace function is extremely dangerous and should be labeled as such.

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.