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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Title: Coder is broken » Drupal 8 is broken on coder HEAD
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Active » Needs review
FileSize
2710219.5.patch4.59 KB

Here's a patch that reverts the breaking change.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Alternatively you can just do git revert d39172e

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Needs work » Needs review
FileSize
2710219.10.patch1.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

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

But then we lose test coverage - right?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

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

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi as a volunteer commented
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.