Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is a utility module intended to include a collection of scripts useful for batch processing contents on a Drupal site.
Project Link: http://drupal.org/sandbox/kenyangzj/1904474
git clone --recursive --branch master kenyangzj@git.drupal.org:sandbox/kenyangzj/1904474.git quick_fix
This is a Drupal 7 module.
Comments
Comment #1
ycshen CreditAttribution: ycshen commentedIt 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:
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.
Source: http://ventral.org/pareview - PAReview.sh online service
Comment #2
kenyangzj CreditAttribution: kenyangzj commentedSwitched to branch 7.x-dev and deleted master branch.
Fix some coding styles (spaces, tabs ...)
Comment #3
strakkie CreditAttribution: strakkie commentedHi kenyangzj,
The new git branch isn't correct, you could work in 7.x-1.x for example, not in 7.x-dev.
Also see,
http://ventral.org/pareview/httpgitdrupalorgsandboxkenyangzj1904474git
A lot of errors are shown here because you are using tabs in your code, replace these with spaces to remove most of the shown errors. The other errors shown here are mostly formatting errors, try to find these errors on the lines provides by the webpage.
A lot of the conventions for coding standards can be found here:
http://drupal.org/coding-standards
Comment #4
strakkie CreditAttribution: strakkie commentedHad some time to look into the code, you should have a look at the following:
When there are no nodes in the selected content types, the following warnings show:
An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /review/batch?render=overlay&id=3&op=do StatusText: OK ResponseText: Fatal error: Call to a member function claimItem() on a non-object in C:\xampp\htdocs\review\includes\batch.inc on line 274
And after going back:
Notice: Undefined variable: batch_ops in quick_fix_landing_submit() (line 59 of C:\xampp\htdocs\review\sites\all\modules\review\quick_fix\includes\quick_fix_forms.inc).
This is because the batch_set function is always called in quick_fix_forms.inc (line 58).
By the way, your module description says:
A utility module that includes a collection of scripts useful for batch processing contents on a Drupal site.
I can only find the "Node title - HTML entity decode" script. Are you planning on creating extra functions for your module?
Comment #5
kenyangzj CreditAttribution: kenyangzj commentedCoding style now passed the automated check.
Addressed issues mentioned in #4
@strakkie You are correct, currently only Node title - HTML entity decode is available. Will plan to include more functions. Suggestions are always welcome.
Comment #6
a_thakur CreditAttribution: a_thakur commentedAs installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page.
This project is too short to approve you as git vetted user. This discussion elaborates, how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project.
Comment #7
adnasa CreditAttribution: adnasa commentedquick_fix_forms.inc
Did this statement really worked? o_O
Here's a suggestion (and recommended):
Best of luck!
/ adnasa
Comment #8
AolPlugin CreditAttribution: AolPlugin commentedPretty useful module, like the idea!
A few issues, though:
1. If you select a content type which has no nodes in it, nothing is done. A warning should appear.
2. In includes/quick_fix_forms.inc on line 44, it's better to either use the code in #7 or db_select().
3. in quick_fix.module on line 8, it's best to use module_load_include
Comment #9
kenyangzj CreditAttribution: kenyangzj commentedThanks both #7 and #8. I have included the changes into the 7.x-dev branch.
@AolPlugin just curious, why module_load_include is preferred compared to require_once?
By the way, currently this module has only one functionality to html decode node titles and I'm look for suggestions/ideas on what to add. Feel free to suggest :)
Comment #10
AolPlugin CreditAttribution: AolPlugin commentedmodule_load_include is nothing more but a small utility function from Drupal, but it might come in handy sometimes (as per http://stackoverflow.com/questions/7270729/module-load-include-vs-require-once).
If you look over the source code (here), you will see that it includes the full path to the file, and checks for it's existence. If it fails it returns
FALSE
, which you can then handle. In your case, you know that it exists, but I was suggesting it for future references.Comment #11
sreynen CreditAttribution: sreynen commentedComment #12
seworthi CreditAttribution: seworthi commentedComment #13
seworthi CreditAttribution: seworthi commentedThis is a review of the 7.x-1.x
The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.
http://ventral.org/pareview/httpgitdrupalorgsandboxkenyangzj1904474git
This is a security blocker. Other than this issue, the rest of the module is good.
Comment #14
PA robot CreditAttribution: PA robot commentedClosing due to lack of activity. Feel free to reopen if you are still working on this application.
I'm a robot and this is an automated message from Project Applications Scraper.