Update Coder to 8.3.28

It seems like we can now remove Drupal.Commenting.VariableComment.Missing everywhere we have an additional ignore because this looks to be handled correctly now - but not sure if doing that is really in scope here or if we just do it later.

Current changes look clean and nothing stands out as weird.

If we can remove more ignores we could, but might as well be in a followup.

3.29 was released should we use that?

I tested with 3.29 locally and now all the phpcs:ignore Drupal.Commenting.FunctionComment.MissingReturnComment lines are ignored and phpcs is reporting errors when previously it was not.

Created 🐛 [regression] Can no longer ignore Drupal.Commenting.FunctionComment.MissingReturnComment Active .

I also asked about this two days ago in #coder but there hasn't been a response yet.

This is the changelist:
https://git.drupalcode.org/project/coder/-/compare/8.3.28..8.3.29?from_p...

There are changes to that sniff it seems.

I would suspect: https://git.drupalcode.org/project/coder/-/commit/78034f0a41956c4f8d3808...

While testing the latest Coder dev version on core I detected another phpcs ignore inconvenience and fixed it in 📌 Allow phpcs:ignore before param comments Active .

Found one more Coder issue: 🐛 Compley array<> expressions should be allowed in @return comments Active .

I think we should upgrade straight to Coder 8.3.30 here (not released yet).

I started to work through the newly reported issues and also removed deprecated sniffs. This will not pass yet, but let me know if you like the approach.

I tested with a git checkout of Coder in the vendor directory, to ensure I'm running on the latest version of Coder. Once everything is passing here as we want it I can make a new Coder release.

One nice find with the improved concat sniff: this exposed a bug in test code, yay! The test used ".." but should be "." https://git.drupalcode.org/project/drupal/-/merge_requests/12018/diffs#6...

@klausi, I am going to work on this for a bit. I hope we don't duplicate work.

Sure, I'm currently not working on it!

phpcs is passing now. The only failing test is core/tests/Drupal/BuildTests/Composer/Template/ComposerProjectTemplatesTest.php which is expected since this is using a dev version of Coder.

Nice, looks all good to me.

Then I will make a new Coder release now and we can use that.

Coder 8.3.30 released!

https://www.drupal.org/project/coder/releases/8.3.30 →

Please update that in composer, then we can move to RTBC I think.

For completeness only, I am adding that this is adding return descriptions for cases that were deliberately ignored in previous issues, such as, 📌 Fix Drupal.Commenting.FunctionComment.MissingReturnComment in core/lib/Drupal/Core/Config Active . I think the work to add the descriptions could have been deferred because having comments alongside an inheritdoc is a violation of our coding standards. But is it done now and hopefully the code is better for it.

Thanks, looks good to me except drupal/core-recipe-unpack being added to composer.lock. Can you revert that?

Agreed with not renaming these classes - these are deprecated anyway so will be removed in Drupal 12, if they weren't, that should still happen in a dedicated issue rather than here.

There are conflicts in composer files now.

Rebased.

Hm, please don't do rebases in merge requests. Now it is very hard for me to review your changes you added. Always do merges form the main branch in merge requests.

composer.lock looks good now, and if I interpret the rebase correctly there are no other changes. Looks ready!

@klausi I usually rebase merge requests because merges make future rebases (and sometimes merges) harder. Although if a merge commit has already happened the only option is another merge commit.

Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!

catch → closed merge request !12018

This does actually go in the release notes, and we do need a release note since the update changed a bunch of phpcs rules.

Does this need a 10.5.x backport?

Added release note snippet.

It's not that important to backport, but I guess we could to keep everything in sync?

Correct branch.

I tried:

1. composer update drupal/coder drupal/core squizlabs/php_codesniffer
   

2. Manually copying over the changes from the 11.2.x version of phpcs.xml.dist.

3. vendor/bin/phpcbf -ps --standard=core/phpcs.xml.dist
   

Sadly, the result is "No fixable errors were found." If I run phpcs, Generic.Strings.UnnecessaryStringConcat has hundreds of errors and is not remotely close to passing on D10. So, I restored the old Drupal rule instead.

Weirdly, none of the phpcs:ignore jazz seems to be necessary on D10. I guess we have added those in the course of D11-only cleanups or something?

Anyway, I believe the MR will pass now unless I broke something else.

Composer changes look legit; confirmed that the phpcs.xml.dist in 11.2.x appears to match the changes made here.

catch → closed merge request !12380

Committed/pushed to 10.5.x, thanks!

I had to use --no-verify because the pre-commit hooks were hanging, hopefully this is not a real problem.

Thanks everyone!

I added a 10.5.x version of the release note excluding the mention of the concatenation rule.