[D7] Bulk Delete Nodes

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxarthgwyr2022121git

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.

codesniffer review appears to be mostly formatting issues which need to be cleaned up. http://ventral.org/pareview/httpgitdrupalorgsandboxarthgwyr2022121git

Basic test of functionality worked properly, nodes were deleted as expected.

There is some commented out code which should probably be removed
bulk_delete_nodes.inc around lines 246, 378, 433

All seems fine. Nice module.

Even though the module is now in RTBC status, here is a little comparison with similar approaches:

I'd go for the VBO if I had to make a decision, but users with less Views and VBO experience will definitely find this module very useful!

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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:
The feature, UI and use case of this module can be built with Views Bulk Operations without writing a single line of code. The View can be exported to a PHP include file providing the default View to delete content as in your example. So instead of duplicating code that will have bugs you should rely on existing projects. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. So my suggestion is to create a Views configuration with the exposed filters and bulk operations available as you need them, export them to code and commit them. There will not be much code left after that.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

+1 for Klausi's comments.
A feature module would be the best way I guess (so we can add dependencies, the View and make it easy to update, etc).

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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

Added review

Hi all,

I'm contributor in this module and would like to correct bugs and release the module.
The key point of this module is the simplicity. It was design for users that have had no past experience with views, views bulk operations, features and are just admins which are working with content. In my opinion there is no need to add redundant code for views and vbo, as far as I'm using drupal SQL API and there is no any security issues.
Thanks in advance.

Git errors:

• There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/bulk_delete_nodes.inc
  --------------------------------------------------------------------------
  FOUND 3 ERRORS AFFECTING 3 LINES
  --------------------------------------------------------------------------
   159 | ERROR | [x] Whitespace found at end of line
   171 | ERROR | [x] Line indented incorrectly; expected at least 2 spaces,
       |       |     found 0
   172 | ERROR | [x] Line indented incorrectly; expected at least 2 spaces,
       |       |     found 0
  --------------------------------------------------------------------------
  PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  --------------------------------------------------------------------------
  

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/bulk_delete_nodes.inc
  ----------------------------------------------------------------------
  FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
  ----------------------------------------------------------------------
   122 | WARNING | Unused variable $types.
   348 | WARNING | Unused variable $languages.
  ----------------------------------------------------------------------
  

• 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:

• bulk_delete_nodes_confirm_form(): never concatenate variables directly into SQL strings as that can easily result in SQL injection security vulnerabilities. Always use placeholders with the DB API functions, for example the where() method has a second parameter for all arguments that should be replaced in the query string. See https://api.drupal.org/api/drupal/includes!database!select.inc/function/...

Otherwise looks good to me, but placeholder usage in SQL is a blocker right now.

Thanks for review.

Currently I'm working on 7.x-1.x branch but there is no option to change default branch(see the screenshot).
I've commited changes and have changed db query where statement to

condition()

function.

I had a look at your module and here are my thoughts:

User Interface

• The module is installed in 'Content' module group. You should not use the 'package' field in your .info file unless you provide many modules that work together, like Devel or Chaos Suite.
• You don't provide help for your module.
• The admin form for the module is a bit cryptic: What is 'Batch loop size'? The users of your module might not be programmers, and even for them, this really doesn't seem to mean much. You should give this field a better name and provide a description to explain what it means.
• The field names in the Bulk Delete forms are all not capitalised. You should capitalise all field names.
• Your interface would be cleaner if date filter was not hidden initially. You're only hiding two date fields, so de-cluttering aspect is negligible, and users don't immediately see what they can do. I think that you should show the dates right there in the fieldset (preferably not inside a separate container if you can avoid it), and provide an extra check box to enable or disable the date filter.
• You should change the field names inside the Delete Mode to something like 'immediate' and 'scheduled' and provide a better description of how it's going to work. You only say that it could happen later using Drupal Queue API, but you don't say that it would be on the next cron run. I think that you should say that when using scheduled delete mode, the nodes will be deleted on the next cron run.
• After adding the filter to delete nodes, it would be worth listing the nodes the user is about to delete. I'm sure you could come up with a way to deal with a huge number of nodes to show (for example, list first 20 and show a 'more' link).
• The message shown after user saves configuration is missing the full stop ('Your changes have been saved'). Same goes for the form validation error messages ('Not a valid batch limit' and 'Not a valid queue time').

Functionality

• You haven't written automatic tests for your module. It's very important that you implement them so that you can test the module on different environments and after you roll out some changes.
• The batch progress is stuck at 0 and no nodes are deleted if 'Batch loop size' is set to 0. You should validate it to be at least 1.
• Overall, I think that the task of this module could be accomplished more elegantly if you just provided good filters for the Content list page. If users could use your filters on that page, they could do a lot more than just delete with the selected nodes.

Code

bulk_delete_nodes.module

• The constants don't have docblocks, or even inline comments describing what they do.

bulk_delete_nodes.inc

• I don't like how you use '#prefix' and '#suffix' to surround the form elements with a div. You should use the theme function to do it. This way you could avoid having an extra div around the form element, and just add a class to the outer div.
• The user list in the 'user' filter shows an empty value for uid 0.
• You can make these few lines a lot shorter and more readable by using shorthand array notation as present in many places in core:
  

  if (!isset($form_state['storage'])) {
    $form_state['storage'] = array();
  }
  $form_state['storage']['nids'] = $nids;
  // is equivalent to:
  $form_state['storage]['nids'] = $nids;
  

• The message in bulk_delete_nodes_form_validate() (line 275) is missing a full stop ('You should choose some filters'). Also it's a bit unclear. I think it should say 'Please specify at least one filter.'
• You should indent the array entry on line 355.
• The $array variable is not named well. You should call it for what it represents, like $user or $record or something.

Overall, it's cute and does what it says on the tin, but definitely requires more work. I hope you find my comments helpful.

tiko wrote:

Currently I'm working on 7.x-1.x branch but there is no option to change default branch

It doesn't look like your sandbox - it belongs to artur.martirosyan

Even if you have permission to commit to the repo, you may not have permission to change branches.

Check out the permissions with artur.martirosyan

Forgot to change the status!

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.