Hi,

Downloaded Module, Works great. There is issue with following with code standards. Please follow https://www.drupal.org/coding-standards

Reviewed module with Coder module. Found warnings, statements in some of the files in nodequeue module.

CommentFileSizeAuthor
#9 nodequeue_codereview-2748919-9.diff85.36 KBwingmanjd
#4 nodequeue_codereview-38678-1.patch52.39 KBharish b
#3 Code review smartqueue.png544.68 KBharish b
#3 Code review nodequeue_generate .png535.24 KBharish b
#3 Code review nodequeue .png1.9 MBharish b
#2 nodequeue_codereview-38678-1.patch52.39 KBharish b
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

harish b created an issue. See original summary.

harish b’s picture

harish b CreditAttribution: harish b at Srijan | A Material+ Company for Srijan | A Material+ Company commented
FileSize
nodequeue_codereview-38678-1.patch52.39 KB
harish b’s picture

harish b CreditAttribution: harish b at Srijan | A Material+ Company for Srijan | A Material+ Company commented
FileSize
Code review nodequeue .png1.9 MB
Code review nodequeue_generate .png535.24 KB
Code review smartqueue.png544.68 KB
harish b’s picture

harish b CreditAttribution: harish b at Srijan | A Material+ Company for Srijan | A Material+ Company commented
Issue summary: View changes
FileSize
nodequeue_codereview-38678-1.patch52.39 KB

Here is the Revised patch(comment#1), which remove bugs, warnings and statements followed according to drupal standards.

Please review it.

Status: Needs review » Needs work

The last submitted patch, 4: nodequeue_codereview-38678-1.patch, failed testing.

Manjit.Singh’s picture

Manjit.Singh
Primary language Hindi
Location Chandigarh
CreditAttribution: Manjit.Singh at Srijan | A Material+ Company commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +Coding standards

@harish I have reviewed the patch and looks good. All changes are related to coding standard.
And regarding the automated test fails on php 5.3, I think we can ignore that because latest version is supported until 5.4 or higher , so no need to take a worry about PHP5.3. Please check this for more information. https://www.drupal.org/requirements
moving it to RTBC.
Thanks @harish for the contribution and too long patch. :)

fizk’s picture

fizk CreditAttribution: fizk commented
Status: Reviewed & tested by the community » Needs work

Thanks for beginning the clean up effort. We still have a long way to go:

phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme . | grep ERROR | wc -l

shows there are 762 errors.

phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme . | grep ERROR | wc -l

shows there are 102 warnings.

wingmanjd’s picture

wingmanjd CreditAttribution: wingmanjd commented

I'd be happy to help on this. Would it be better to submit one large patch for everything, or smaller patches?

wingmanjd’s picture

wingmanjd CreditAttribution: wingmanjd as a volunteer commented
FileSize
nodequeue_codereview-2748919-9.diff85.36 KB

Here's some work so far.

zkhan.aamir’s picture

zkhan.aamir CreditAttribution: zkhan.aamir at Specbee for Drupal India Association commented
Assigned: harish b » Unassigned