[D7] Global Regex

Git clone failed for http://git.drupal.org/sandbox/neelamchaudhary/2673038.git while invoking http://pareview.sh/pareview/httpgitdrupalorgsandboxneelamchaudhary267303...

Git clone failed. Aborting.

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.

Please add your manual review of project applications. Check https://www.drupal.org/node/1975228 , It will speed up your project application review.

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

Edit: removed long pareview.sh dump.

Please add your manual reviews till then.

Thanks for fixing pareview errors, However I have found one potential issue in coder.

I also see this notice when creating or editing a cck field.

Looks good to me!
Thanks for the hard work @neelam.chaudhary

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Can you please confirm if your module is providing the diffrent functionality from these. views_regex_rewrite .Because there is lot of problems of duplication and/or fragmentation on drupal.org

Master Branch

Yes: Follows the guidelines for master branch.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

No: I couldn't found any README.txt file in your module. Please add.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No: Does not Meets the security requirements.

$form['name'] in function global_regex_add_form is not properly sanitized. If I enter <script>alert('XSS');</script> in the name field of admin settings, I will get a nasty javascript popup. You need to sanitize user provided text before printing, Please check https://www.drupal.org/node/28984 and https://api.drupal.org/api/drupal/includes%21common.inc/group/sanitizati... . Also check the screenshot.

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

Forget to attach screenshot.

Forget about README.txt, I had not pulled the latest code base.

This looks like it duplicates https://www.drupal.org/project/fapi_validation

Git errors:

• Git default branch is not set, see the documentation on setting a default branch.

manual review:

1. project name: why "global regex"? The regex is not global, it is local to a field? Can you come up with a better project name that describes what the module does? Like "field_regex" or something?
2. Can you add the differences to existing projects like https://www.drupal.org/project/fapi_validation to the project page?
3. Why does the project page talk about CCK? That does not exist in Drupal 7 terminology?
4. global_regex_add_form(): node_type_load() is used here as #machine_name callback, copy and paste error?
5. global_regex_add_form(): do not call theme here, just add to the form render array. Drupal core will render it later for you. Same in global_regex_list(), do not call theme() here, just return a nested render array. See https://www.drupal.org/node/930760
6. global_regex_add_form_submit(): check_plain() is wrong here. Form https://www.drupal.org/node/28984 : "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"

The wrong usage of check_plain() is a blocker right now.

As far as I can see there was a security issue because global_regex_list() does not filter $type->name, which is the human readable name that can contain dangerous characters. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, no review comments linked in the issue summary. Make sure to read through the source code of the other projects, as requested on the review bonus page.

> And with global regex regex based validations on text fields created with CCK can be applied without implementing form_alter and regex based validation rules can be created without any coding only with admin forms. Additionally these rules can be used in code as well.

So what you're saying is that fapi_validation is an API, aimed at developers, whereas your module offers a UI for site builders?

Maybe then it would be a good idea to have your module build on top of fapi_validation. fapi_validation already supports regexes, as well as lots of other validation rules, and is extensible to provide other rules, and works with https://www.drupal.org/project/clientside_validation. Building a UI on top of that would provide more value overall to the Drupal ecosystem.

Please add this tag when you adding the manual code reviews.

Git default branch is not set.

Please check below for more information:
http://pareview.sh/pareview/httpgitdrupalorgsandboxneelamchaudhary267303...

I have checked code at http://pareview.sh/pareview/httpgitdrupalorgsandboxneelamchaudhary267303... and found some issue as per Drupal coding standards (please check the Drupal coding standards).

Please check attached file (2673038.git-pareview.sh_.png) also.

Thanks @Neelam.

I have reviewed code as per 'Drupal coding standards'. These code are fine and also reviewed functionality for this module.
Module working fine :)

manual review:

1. global_regex_schema(): do not call st() for database table descriptions, this only creates overhead for translators.
2. global_regex_schema(): why does the global_regex table have a serial id column? I think the machine_name should be the primary key?
3. global_regex_menu(): instead of using just "%" for your paths you can use "%global_regex_type", then your page callbacks get the loaded array directly. See Auto-Loader Wildcards at https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
4. global_regex_field_attach_validate(): doc block is wrong, this is hook_field_attach_validate().

Otherwise looks good to me.

Thanks for your contribution, Neelam!

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.