Running coder on HEAD results in PHP Fatal error: Uncaught exception 'PHP_CodeSniffer_Exception' with message 'Referenced sniff "Drupal.Functions.FunctionDeclaration.SpaceAfter" does not exist' in /Users/alex/.composer/vendor/squizlabs/php_codesniffer/CodeSniffer.php:1121

The commit
* d39172e - Removed function declaration sniff, now covered by upstream Squiz sniff (3 days ago)
broke stuff.

CommentFileSizeAuthor
#10 2710219.10.patch1.5 KBalexpott
#5 2710219.5.patch4.59 KBalexpott
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Priority: Critical » Normal

Ah I see... Drupal 8 only works with 8.2.7 because we've got Drupal.Functions.FunctionDeclaration in our phpcs.xml.dist

Shouldn't dropping rules only occur in major versions?

alexpott’s picture

Title: Coder is broken » Drupal 8 is broken on coder HEAD
klausi’s picture

Now that people use phpcs.xml files all over the place we should not drop classes anymore. Let's put back an empty class, mark it as deprecated and remove it in a future 9.x version.

alexpott’s picture

Status: Active » Needs review
FileSize
4.59 KB

Here's a patch that reverts the breaking change.

alexpott’s picture

Alternatively you can just do git revert d39172e

klausi’s picture

Status: Needs review » Needs work

Nah, I would rather like to rely on upstream PHPCS code which also comes with a fixer.

Empty class should be enough, right?

alexpott’s picture

That's a breaking change because people with it in phpcs.xml are going to assume it is testing something.

alexpott’s picture

Maybe we can just extend from the Squizz class...

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.5 KB

Here's a way of fixing it whilst still maintaining the sniff. It does mean we get some double testing but that's going to be necessary if we want the improvements that the change included.

klausi’s picture

Hm, another approach would be to restore the sniff and disable it ruleset.xml. That way we can deprecate the class while leaving it intact and we avoid double errors reported.

alexpott’s picture

But then we lose test coverage - right?

klausi’s picture

I don't think so, since the only test coverage for this old sniff is in bad.php. That is tested with all rules defined in ruleset.xml.

  • klausi committed 340ea50 on 8.x-2.x
    Issue #2710219 by alexpott, klausi: Restore Drupal.Functions....
klausi’s picture

Title: Drupal 8 is broken on coder HEAD » Restore Drupal.Functions.FunctionDeclaration sniff for compatibility with existing phpcs.xml configs
Status: Needs review » Fixed

Committed that approach.

Status: Fixed » Closed (fixed)

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