Hi,

I'm working on an enterprise Drupal project in the UK, and I'm currently integrating Superfish. We love the module, it's going to save us a lot of time, but where possible we like to bring stuff up to Drupal coding standards (it makes our automated tools less noisy). So I ran Superfish under the Drupal code-sniffs, and fixed a whole bunch of minor formatting errors. There are a few issues left (line-length, mostly), but I thought I'd start with the easy stuff.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dotton’s picture

Patch against the head of 7.x-1.x attached.

Eli-T’s picture

Running

phpcs . --standard=Drupal --extensions=module,install,inc

before patch:

FILE: /home/vagrant/temp/superfish/superfish.admin.inc
--------------------------------------------------------------------------------
FOUND 4 ERROR(S) AFFECTING 4 LINE(S)

FILE: /home/vagrant/temp/superfish/superfish.drush.inc
--------------------------------------------------------------------------------
FOUND 11 ERROR(S) AND 3 WARNING(S) AFFECTING 9 LINE(S)

FILE: /home/vagrant/temp/superfish/superfish.install
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)

FILE: /home/vagrant/temp/superfish/superfish.module
--------------------------------------------------------------------------------
FOUND 109 ERROR(S) AND 25 WARNING(S) AFFECTING 112 LINE(S)

After patch:

FILE: /home/vagrant/temp/superfish/superfish.install
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------

FILE: /home/vagrant/temp/superfish/superfish.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------

so this looks really good.

One query, after patch, at line 1760

/**
 * Helper function that builds the nested lists of a Superfish menu.
 *
 * @param array $variables
 *   .
 *
 * @return array
 *   .
 */

I don't agree with putting the . instead of an actual description just to get rid of the warning.

Apart from that I can confirm this patch contains no functional changes.

Eli-T’s picture

Status: Needs review » Needs work
dotton’s picture

Agreed. I'm not clear on what the values in the $variables array do, so I've stripped the error-suppressing dots.

dotton’s picture

Status: Needs work » Needs review
Eli-T’s picture

Assigned: dotton » Unassigned
Status: Needs review » Reviewed & tested by the community

OK - I've just run PHPCS againt the new patch and two errors are returned but that's much better than artificially suppressing them.

Patch looks good.

mehrpadin’s picture

Hey everybody,

Thanks for this :) please check the development release once updated, note I haven't exactly used the patch, but followed everything, gives me no error, so should be fine, thanks again!

Eli-T’s picture

Looks like there's still no blank line at the end of the file. Apart from that looks good in latest dev.

mehrpadin’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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