[D7]Bulk delete 301

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.

Automated Review

According to pareview.sh (http://pareview.sh/pareview/httpgitdrupalorgsandboxalokvermaei2330429git), I got 2 problems.

1. README.txt file missing.
2. LICENSE.txt exist and need to remove.

Manual Review

Individual user account

Yes.

No duplication

Yes

Master Branch

No

Licensing

Yes: Follows the licensing requirements

3rd party code

No

README.txt/README.md

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

Code long/complex enough for review

No

Secure code

Yes

Please remove the LICENSE.txt file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.

Please use README.txt instead of readme.txt

Automated Review

http://pareview.sh/pareview/httpgitdrupalorgsandboxkaran2246897git-7x-1x found a list of problems. Recommend taking a close look.

This modules looks nice to me and as per Drupal standards

However I have some suggestions:
1) When a tried more than single character delimiters, it gave me “Notice: fgetcsv(): delimiter must be a single character in bulk_delete_301_read_file() (line 43 of D:\xampp\htdocs\drupal\sites\all\modules\bulk_delete_301\bulk_delete_301.module).” for each line in file.
You must restrict/suggest user to provide only single character delimiter. PS below for details.

2) In function “bulk_delete_301_read_file()”, why do we overwriting the array values. Instead of doing this I would suggest you to provide default values (may be using constants).

NB: Moving this to Needs work to address minor issue, which are off course no show stoppers :)

Updated issue summary.

Hi alokvermaei,

If this is the case and patch required to omit those notices, I would recommend you to edit your project page and state it very clearly that "to use more than 2 characters as delimiters you should apply this page".

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Thanks Again!

@alokvermaei, Thanks for the contribution.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder is fine. http://pareview.sh/pareview/httpgitdrupalorgsandboxalokvermaei2330429git

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

No: 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. If "no", list security issues identified.

Coding style & Drupal API usage

1. bulk_delete_301.admin.inc bulk_delete_301_form_validate : ini_set('auto_detect_line_endings', TRUE); I am not sure regarding the setting of ini in the .module file.if its the requirement of the module then specify on the project page.
2. bulk_delete_301.module bulk_delete_301_read_file : Instead of calling bulk_delete_301_data function for every single record insert.Create an array of objects of inserted items and insert in a single shot and by moving the code of bulk_delete_301_data
   

   $redirect = (object) $data;
     if (strpos($redirect->source, 'node/') !== FALSE) {
       $path = $redirect->source;
     }
     else {
       $path = drupal_lookup_path("source", $redirect->source);
     }

   into the isbulk_delete_301_read_file. This will make the operation fast in the case of CSV files with huge data.

This review uses the Project Application Review Template.

You should fix above issues first and get a review bonus by manual reviews of another 3 modules as you have already taken the review of two modules so take review of one more module that would help speed up the process and this will put you on the high priority list, then git administrators will take a look at your project right.

Another point :
I have reviewed your commits and found that is committed by unknown.Configure git properly on your system.See Maintaining a drupal.org project with Git

Thanks Again!

This sounds like a good feature that should be added in the existing Redirect project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the Redirect issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward, as currently Seeking co-maintainer(s) as well. If you cannot reach the maintainer(s) please follow the abandoned project process.

Assigning to myself for next review.

Being a git review administrator before reviewing any module, I assigned the issue to myself :)

See the review process workflow https://www.drupal.org/node/539608 again.

As I was about to start your project review, found README.txt so weird. Better to look into guidelines for in-project documentation and the README Template again.

Thanks!

@alokvermaei

Why CSV? Why not to use a simple textarea field? This way you could avoid all troubles related to working with CSV files.

This is my first time so i havn't idea about the work flows

This is an everybody's first time, because it's an one-time procedure.

FWIW I'm inclined to agree with pingwin4eg that CSV seems unnecessarily complicated if we're talking about a single column of values.

I had a look at this module in a vanilla D7 install running PHP 5.5 and hit an interesting error:

PHP Fatal error: Unsupported operand types in /drupal-7.32/sites/all/modules/contrib/bulk_delete_301/bulk_delete_301.module on line 33

...this is before I tried to upload a file.

The stack trace for this error looked strange:

PHP Stack trace:
PHP   1. {main}() /drupal-7.32/index.php:0
PHP   2. menu_execute_active_handler() /drupal-7.32/index.php:21
PHP   3. drupal_deliver_page() /drupal-7.32/includes/menu.inc:532
PHP   4. drupal_deliver_html_page() /drupal-7.32/includes/common.inc:2557
PHP   5. drupal_render_page() /drupal-7.32/includes/common.inc:2669
PHP   6. drupal_render() /drupal-7.32/includes/common.inc:5760
PHP   7. theme() /drupal-7.32/includes/common.inc:5898
PHP   8. theme_render_template() /drupal-7.32/includes/theme.inc:1201
PHP   9. include() /drupal-7.32/includes/theme.inc:1517
PHP  10. render() /drupal-7.32/themes/seven/page.tpl.php:28
PHP  11. drupal_render() /drupal-7.32/includes/common.inc:5999
PHP  12. drupal_render() /drupal-7.32/includes/common.inc:5905
PHP  13. drupal_render() /drupal-7.32/includes/common.inc:5905
PHP  14. drupal_render() /drupal-7.32/includes/common.inc:5905
PHP  15. theme() /drupal-7.32/includes/common.inc:5898
PHP  16. bulk_delete_301_process_file() /drupal-7.32/includes/theme.inc:1122

It turns out that the name of the bulk_delete_301_process_file function means it's being selected as a theme process_function for the file element in the bulk_delete_301_form and this means it's being called when that form is rendered, with unexpected values passed to it. Some debug in the (pre)process hook code in theme.inc showed this as the value for $info within theme() when the bulk_delete_301_process_file is called resulting in the fatal error:

Array
(
    [render element] => element
    [type] => module
    [theme path] => modules/system
    [function] => theme_file
    [process functions] => Array
        (
            [0] => bulk_delete_301_process_file
        )

)

So I think at the very least you'll need to rename that function so that it doesn't get called unexpectedly by the theme system.

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

• 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: so this module is only useful for redirect.module users, right? That should be added to the project page, see https://www.drupal.org/node/997024 for further suggestions.
2. So bulk_delete_301_processing_file() and bulk_delete_301_data() will not scale for a large CSV file with many entries. Did you consider using the Batch API to process the rows? This will probably run into a PHP timeout if the dataset is large.
3. "filter_xss(t('Line @line_no: The source "@source" does not exist in redirect table.'...": filter_xss() is wrong here since all dynamic parts are already sanitized with the "@" placeholder in t(), so there is nothing to filter. Make sure to read https://www.drupal.org/node/28984 again to know when to use filter_xss() and when not.
4. bulk_delete_301_processing_file(): doc block does not tell much. It should describe what the function does, something like "Reads URLs from the CSV file and deletes redirects.". See https://www.drupal.org/coding-standards/docs#functions . Same for bulk_delete_301_data() what is $data and $options? @param docs missing, see https://www.drupal.org/coding-standards/docs#param

But that are not application blockers, otherwise looks good 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.

No objection for more than a week, so ...

Thanks for your contribution, alokvermaei!

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.