[D7]Metatag Replace

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

Fixed the git clone URL in the issue summary for non-maintainer users.

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

Hi @namit.garg,

Automated Review

No issues reported by preview.sh

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

 

No: Does not follow the guidelines for in-project documentation and/or the README Template. I think a little more introduction is needed, in particular, you need to address this concern from the Module Documentation Guidelines: “It should always start with a synopsis, telling users what the project does, what problem it solves and the intended audience is”. I would also suggest formatting the project page html correctly, with heading level tags and correct list tags.

The README file doesn’t quite follow the README template as laid out on drupal.org: README template.

Implement hook_help(), see Help text standard for more info.

.

 

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

 

     
1. * I don't believe you have implemented hook_menu correctly. The admin page is not appearing in the 'Search and Metadata' section.
2. * Your note in 'Limitation' that "Attempting to in two separate windows can lead to unknown errors." needs to be addressed.
3. * Running a search consistently produced the following errors:
   
   Notice: Undefined index: opn in metatag_replace_finish_batch_search() (line 511 of /home/r00e0/www/sites/default/modules/2623618/metatag_replace_admin.inc).
   
   Notice: Undefined index: search-word in metatag_replace_finish_batch_search() (line 512 of /home/r00e0/www/sites/default/modules/2623618/metatag_replace_admin.inc).
   
   Notice: Undefined index: replace-word in metatag_replace_finish_batch_search() (line 513 of /home/r00e0/www/sites/default/modules/2623618/metatag_replace_admin.inc).

   

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.

Automated Review

No issue reported by preview.sh

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: follow] 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. / No: List of security issues identified.]

Coding style & Drupal API usage

Following errors observed when I searched for decription meta tag:

Notice: Undefined index: field_result in include() (line 31 of /Applications/MAMP/htdocs/fresh-drupal/sites/all/modules/metatag_replace/metatag_replace.tpl.php).
Notice: Undefined index: matches in include() (line 37 of /Applications/MAMP/htdocs/fresh-drupal/sites/all/modules/metatag_replace/metatag_replace.tpl.php).
Notice: Undefined index: node_title in include() (line 38 of /Applications/MAMP/htdocs/fresh-drupal/sites/all/modules/metatag_replace/metatag_replace.tpl.php).
Notice: Undefined index: nid in include() (line 39 of /Applications/MAMP/htdocs/fresh-drupal/sites/all/modules/metatag_replace/metatag_replace.tpl.php).
Warning: Invalid argument supplied for foreach() in include() (line 40 of /Applications/MAMP/htdocs/fresh- drupal/sites/all/modules/metatag_replace/metatag_replace.tpl.php).

Needs work.

Hello,
Please look into following error which i have found :
1. Please check Drupal coding standard: https://www.drupal.org/coding-standards

FILE: /var/www/drupal-7-pareview/pareview_temp/metatag_replace.module
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
64 | WARNING | Open page callback found, please add a comment before the
| | line why there is no access restriction
---------------------------------------------------------------------------

2. Found some spelling errors in your code.

./metatag_replace_admin.inc:165: Dont ==> Don't
./metatag_replace_admin.inc:180: Dont ==> Don't

Overall thanks for your hard work.

Cheers,

@namit.garg, looks good for me.

Hi,


When i performed a search i had this notices :

Notice: Undefined index: field_result in include() (line 31 of drupal/sites/all/modules/contrib/metatag_replace/metatag_replace.tpl.php).
Notice: Undefined index: matches in include() (line 37 of drupal/sites/all/modules/contrib/metatag_replace/metatag_replace.tpl.php).
Notice: Undefined index: node_title in include() (line 38 of drupal/sites/all/modules/contrib/metatag_replace/metatag_replace.tpl.php).
Notice: Undefined index: nid in include() (line 39 of drupal/sites/all/modules/contrib/metatag_replace/metatag_replace.tpl.php).
Warning: Invalid argument supplied for foreach() in include() (line 40 of drupal/sites/all/modules/contrib/metatag_replace/metatag_replace.tpl.php).

You should perform some checks, maybe for instance replace in metatag_replace.tpl.php on line 37 :

$meta_field = $list[0]['field_result'];

with:

if(isset($list[0]['field_result'])) {
  $meta_field = $list[0]['field_result'];
  ...
}

in fact the problem occurred in #21 when i click directly on "replace" button and my searched metatag is not found

I found an other problem. I dont think it's a good idea to use so general session variables:
$_SESSION['show-result']
you should probably prefix them with your module name.

you have too much logic i think in your metatag_replace.tpl.php file.
Maybe you should split it to parts, maybe one for search and an other for replace...
I saw you were printing directly

Hi namit.garg,

I was going through your code and found that, you have not followed naming convention standards for submit & replace buttons in the form.

Please look into the below code line #130 in metatag_replace_admin.inc.
metatag_replace_form_submit_one() function will confuse the user who is going to use this module in future.

'#submit' => array('metatag_replace_form_submit_one'),

Also for Replace button, please provide appropriate name for button and functions.

 $form['submit1'] = array(
    '#type' => 'submit',
    '#value' => 'Replace',
    '#submit' => array('metatag_replace_form_submit_two'),
    '#validate' => array('metatag_replace_form_validate_two'),
  );

Pareview.sh is fine with your module.

From manual checks I did find the following:
- Your tpl files contain PHP logic that should be handled outside of these files, using only the data coming from functions within your module rather than the tpl itself.
- Some code style issues, like mixing double quotes and single quotes, e.g. in the t() wrapper.
- Not really an error, but do see a lot of repeating logic, like
$replace_result = preg_replace("/\b$searchword\b/i", $replaceword, $meta_list);
This cries for a helper function that can do the multiple preg_replace operations by handling an array.
- Same applies to repeating if/else statements.

manual review:

1. metatag_replace_form(): why do you call drupal_session_start() here? That should not be necessary? Either remove it or add a comment why it is needed.
2. "'description' => 'Description',": all user facing text must run through t() for translation. Please check all your #options strings.
3. metatag_replace_form(): do not call theme() here. Just pass in the pager render array as variable to you theme functions. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
4. metatag_replace_form() instead of using $_SESSION here you could access the values in $form_state if they exist there, right?
5. metatag_replace_operation_search.tpl.php: is not a template at all, so it should be just a theme function in your module file. Same for metatag_replace_operation_search.tpl.php.
6. metatag_replace_perform_replace(): $var1 is bad variable name. What is it? $metatag_string or $description or somehting?
7. metatag_replace_help(): doc block is wrong, this should be "Implements hook_help().". Please check all your hook doc blocks.

There is a security issue in this module and as part of our git admin training I'm assigning this to Manjit so that he can take a look. If he does not get to it I will post the vulnerability details in one week. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Thanks Klausi for assigning the issue.

@Namit you have added some variables in template files that need to be sanitized before using.

 * $list[0] Stores the Following Data
 * $list[0]['search-word']=> Stores the search word.
 * $list[0]['replace-word']=> Stores the replace word.
 * $list[0]['opn']=> Stores operation performed ie Search,Replace,Complete.
 * $list[0]['field_result']=> Stores the Meta field you searched.
 * $list[0]['detail'] => Stores the nid and link of the Node.
 * $list[0][matches]=> Stores the the array for Metatags found.

Rest of things are fine.
@klausi Please correct me if there is anything else.

@namit.garg: Hey, thanks for building on top of Metatag, I'm very happy to see you do this :)

I have one suggestion - add the same code check logic to the 'replace_box' field that you have for the 'search_box' field, and then I think it'd be a-ok. Also, I think the error message would be better as 'Do not enter any HTML or PHP code.'

If you make this one change I'll be happy to mark it as RTBC.

Ok, the latest changes look good. Good work, namit.garg, that's an interesting module you've put together!

Thanks for your contribution, Namit!

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.

Thanks Namit Garg
For your contribution to Drupal Community