This is the new module for bulk delete operation of 301 redirects by a
csv upload of url's.
It is having another option to save url from 404 if node does not exist.
Steps :
1. Download and extract the module in modules folder (you should have redirect module before installing this module).
2. Enable the Bulk delete 301 redirect module and configure it.
3 . You will get an extra menu item for bulk delete 301 in the redirects menu tab.
4 . Create a csv of urls that you want to remove redirect .
5. There is an option for not deleting the url's which can be 404 after deleting. check the option for "Delete 301 if node exists".

Project Page
https://www.drupal.org/sandbox/alokvermaei/2330429

Git
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/alokvermaei/2330429.git bulk_delete_301

Automated Review
http://pareview.sh/pareview/httpgitdrupalorgsandboxalokvermaei2330429git

Reviews of other projects:
https://www.drupal.org/node/1545640#comment-9187493
https://www.drupal.org/node/2238015#comment-9187567
https://www.drupal.org/node/2342629#comment-9196803
https://www.drupal.org/node/2385341#comment-9463213
https://www.drupal.org/node/2310221#comment-9463285
https://www.drupal.org/node/2392971#comment-9463311

Files: 
CommentFileSizeAuthor
#10 bulk_delete_301.jpg315.53 KBmadhusudanmca

Comments

alokvermaei’s picture

Project: Drupal.org security advisory coverage applications » Bulk delete 301
Component: module » Code
alokvermaei’s picture

Project: Bulk delete 301 » Drupal.org security advisory coverage applications
Component: Code » module
PA robot’s picture

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.

saniyat’s picture

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
saniyat’s picture

Status: Needs review » Needs work
alokvermaei’s picture

Status: Needs work » Needs review

Read me file included in it.

karan_mudi’s picture

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

karan_mudi’s picture

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.

alokvermaei’s picture

LICENSE.txt has been removed and readme.txt file name has been changed to README.txt .

madhusudanmca’s picture

Status: Needs review » Needs work
FileSize
315.53 KB

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.
bulk delete 301
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 :)

alokvermaei’s picture

Status: Needs work » Needs review

HI madhusudanmca ,
thanks for the review,

I have gone through your points let me explain it:
1)You can use more then oneccharacter delimiter if you apply a patch for this please go through
https://bugs.php.net/bug.php?id=38496

naveenvalecha’s picture

Issue summary: View changes

Updated issue summary.

naveenvalecha’s picture

Issue summary: View changes
madhusudanmca’s picture

Status: Needs review » Needs work

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’s picture

Issue summary: View changes
alokvermaei’s picture

Issue summary: View changes
alokvermaei’s picture

Status: Needs work » Needs review

Hi madhusudanmca,

Thanks for your precious time.I have found the same issue with other drupal contributed module for csv upload .I have attached it in the project page as u suggested me. I am also reviewing other project .Recommend me if any further suggestions.

naveenvalecha’s picture

Issue summary: View changes
Status: Needs review » Needs work

@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!

alokvermaei’s picture

Status: Needs work » Needs review

HI naveenvalecha,
Thanks for your review. Please have a look on the below mentioned points.
Point 1: When importing a CSV file, you may get the message "Invalid data value given. Be sure it matches the required data type and format". This usually indicates incompatibility between the line endings in the CSV file and your system, and with PHP 5.3 or later it can be addressed by setting the PHP auto_detect_line_endings option to 1. If it's not convenient to set this in php.ini, you can set it at runtime in your migration constructor. you can refer on this one link https://www.drupal.org/node/1152158

Point 2: Implemented the one time read file in the code .

alokvermaei’s picture

Issue summary: View changes
alokvermaei’s picture

Issue tags: +PAReview: review bonus
alokvermaei’s picture

Priority: Normal » Major
er.pushpinderrana’s picture

Priority: Major » Normal
Status: Needs review » Postponed (maintainer needs more info)

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.

alokvermaei’s picture

Status: Postponed (maintainer needs more info) » Needs review

HI er.pushpinderrana,

Thanks for your valuable response .This module having two added feature with bulk delete redirect (by node as well as alias name) + an option for not deleting the urls becoming a 404 by a simple csv upload. The redirect module is not maintaining these type of Scenario like i have added this feature with redirect like path redirect import and export . If user needs these type functionality then he should use this module only.

er.pushpinderrana’s picture

Assigned: Unassigned » er.pushpinderrana

Assigning to myself for next review.

alokvermaei’s picture

Assigned: er.pushpinderrana » Unassigned

HI er.pushpinderrana,
I can't understand the purpose of assigning yourself. so i am changing it's state to unassigned.

er.pushpinderrana’s picture

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’s picture

Hi er.pushpinderrana ,

Thanks for sharing the workflow process.This is my first time so i havn't idea about the work flows .i will take care of these in future.I have updated the readme.txt file as per the standards.

pingwin4eg’s picture

@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.

mcdruid’s picture

Status: Needs review » Needs work

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.

alokvermaei’s picture

Hi pingwin4eg ,

This module is for bulk delete operation and it's always easy for user to use csv.I am also incorporating the text file with this module so that there should not be any validation for only csv upload.

alokvermaei’s picture

Priority: Normal » Major
Status: Needs work » Needs review

HI mcdruid /pingwin4eg,

i have resolved the function name issue as well as it will work for txt file also Thanks for suggestions. changing the status into review mode.

klausi’s picture

Assigned: Unassigned » kscheirer
Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAReview: review bonus

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.

alokvermaei’s picture

Issue summary: View changes
alokvermaei’s picture

Issue summary: View changes
alokvermaei’s picture

Issue summary: View changes
alokvermaei’s picture

Issue tags: +PAReview: review bonus

HI klausi ,
Thanks for your valuable time and inputs.
point 1: Done
point 2: Trying to create a scenario for php time out and will do it by batch(as it's not a blocker for this ).
point 3:Done.
point 4: Done.

I have also reviewed 3 more project and will continuing for more projects review.
For the time i am making it in review bonus mode.

alokvermaei’s picture

Priority: Normal » Major
klausi’s picture

Assigned: kscheirer » Unassigned
Status: Reviewed & tested by the community » Fixed

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:

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.

alokvermaei’s picture

Thanks klausi and all reviewers for their valuable time.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.