[≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard

Here is a patch which add the single new sniff and removes all others. Also I am going to see if this works, making a change to drupalci.yml to not run the phpunit tests, which take a long time.

Obviously, these big changes are only so we can get quick test results on d.o.. The full set of phpunit tests will need to be run once the bulk of the standards work has been completed in a patch.

Better patch

The patch in #7 ran nice and quickly, 1 min 28s compared to 50 mins for the full phpunit run.
It showed 3283 coding standards messages, but some were not for @deprecated. I took too much out of phpcs.xml.dist - some internal sniffs appear to be run by default even without having a specific include in the phpcs.xml file.

So 946 coding messages is the starting point. Nice that it is under 1000!

Re-rolled.
This patch is not for committing, only to show the extent of warnings in core code.

There are now 948 warning messages for @deprecated tag in core.

However, work on fixing these should be postponed until any possible change in the standard is agreed and the sniff ammended. The standard was all agreed by 13 March at comment #68 on #3024461-68: Adopt consistent deprecation format for core and contrib deprecation messages but since implementaion of the coder rule there has been further disagreements - read from comment #86

Work can re-start on this now that we have agreement on the standards text on #3024461: Adopt consistent deprecation format for core and contrib deprecation messages

Core currently only uses Coder version 8.3.2 so here is a patch which runs the updated sniff (via applying a patch in drupalci.yml).

Now that Core uses Coder 8.3.4 the patch to Coder in drupalci.yml is not required, so here is a simplified version.

Using a starter patch from #3033332: [META] Fix language and consistency of deprecation messages and annotations here is a first attempt at fixing some of the deprecated messages. There are many more to fix.

edit: This patch has reduced the errors and warnings from 960 down to 873, a drop of 87 or nearly 10%

I've done some work to get PHPCS to generate automatic fixes for the @deprecated text - see #3057988: Automatically fixing @deprecated doc text

The patch here updates Coder to have this new functionality and the test run should (hopefully) generate a patch to fix a large proportion of the messages.

Now with the generated phpcs patch added.

Enhanced to cover more common errors in the text. When a @deprecated tag failure is found, the following pattern is tried:
/(as of|in) (Drupal|drupal)( |:)(.+)(,| and) (will be|is) removed (from|before|in) (Drupal|drupal)( |:)([\d\.]+)\.(.*)$/
If this matches, then we know how to correct the text.

Patch generated locally, coder changes also uploaded and applied.

Further enhancements to catch some more common fails. Pattern is

/(as of|in) (drupal)( |:)(.+)(,|\.| and) (will be|to be|is) removed (from|before|in) (drupal)( |:)(.+)\.(.*)$/i

Re-run the base patch to check new total number of warnings and generate new phpcs patch.

Now with the generated fixes

Here's a patch which will run the full test suite. I think it would be good to get these committed, then the next steps will be smaller.

I have checked locally that the patch applies (had to make one change and re-rolled) but I have not run the core tests locally. This processes all the sniffs not just the new one.

So the tests are green and there are 413 warnings remaining (compared to 410 in #20 7 days ago). I have compared the phpcs output and the new ones are only @deprecated so this patch does not introduce any other error.

I'd be happy if this could be committed now, and we tackle the rest in stages. I know that Garbor was also leaning towards that plan in #3048495-39: Fix Drupal.Semantics.FunctionTriggerError coding standard

We shouldn't be committing a fix that results in errors whilst running composer phpcs

Yes you are right, but I was not intending the commit to include the addition of any of the new rules in the first instance. Klausi and I just wanted to get the first 520 fixes done (which is 55% of the whole) so that the manual changes are in a more manageable patch. The manual fixes will take a while and each time it is bound to involve a re-roll of that patch which fixes the 520 because of core commits elsewhere.

OK, here's the same patch, but using the change to phpcs.xml.dixt from #25 to only add the specific Drupal.Commenting.Deprecated.IncorrectTextLayout rule.

Yes, I will work on this. Sorry, I've been busy on other things. If you think that Alexpott's request for ensuring that lines we change here also meet all the other deprecation standards (even though we are ignoring those rules at the moment) then I can alter the automated fixing to try to cover those.

Re-roll of #19 as core 8.8 has moved on a bit. No changes to Coder rules yet, but this will get current baseline and generate new phpcs patch.

898 coding standards errors reported in #32
Here is the patch that was generated.

I had forgotten that the patch file generated by the druapl.org testbot has, for example:

--- tests/Drupal/Tests/UnitTestCase.php
+++ PHP_CodeSniffer

Whereas for it to be actually useful here, it needs to be:

--- a/tests/Drupal/Tests/UnitTestCase.php
+++ b/tests/Drupal/Tests/UnitTestCase.php

Rustled up a quick php script to make the changes on the patch file.

OK, so it also need /core adding in.

New patch to generate a higher proportion of fixes

Fix xml syntax (one ending / not needed)

So eliminating the noise of the other sniffs (missing @see, missing extra-info, wrong url format, which we can tackle subsequently) we have a clearer picture of the basic text to fix: There are 667 'IncorrectLayout' faults from #37, and the results of patch #38 will show the results of the auto-fixing work so far, and indicate what other types of auto-fixing might be possible.

More work done to automatic fixing. This will use a newer patch #8 of coder to generate the fixes patch.

Now run the generated patch.

From #39 we see that there are 668 coding standards messages for Drupal.Commenting.Deprecated.IncorrectTextLayout, and a patch was generated to fix as many as possible.

In #40 this generated patch was run and there are only 58 coding standards messages, so the auto-fixing sorted out 610 of 668 which is 91%.

I'd like to request that this patch is reviewed and if OK then committed, as I do not think the remaining 58 can be fixed automatically. There are many which do not have a deprecation version. Lots can be fixed manually, which I will do after the main patch is committed.

I could then move on to looking at automatically fixing the warnings, but I want to leave that until the majority of the errors (the 610) have been committed.

@gabor, @alexpott, @klausi - what is the next step to getting the changes in #40 reviewed? I know that the actual full patch #40 shoud not be committed because it adds the new sniff. But the standards changes it includes are all ready for review and commit.

Jonathan

Here is the actual patch to commit, i.e without the changes to drupalci.yml and phpcs.xml.dist. This will run the full phpcs (without the new sniffs) so should be clean.

The patch in #40 did not apply due to recent commits on #3069049: Properly deprecate View::fixTableNames and #3052233: Properly deprecate EntityFormInterface::setEntityManager() and trigger an error on use so I have removed those hunks from the patch. I am wary that I'll always be having to re-roll or regenerate this patch as it touches so many files, and even a few days between creation and review will likely mean it no longer applies. But at least I have the process and it is easily repeatable.

Thanks @lendude. Yes, as this currently touches 315 core files it would be nice to get it in, particularly as #3038170: Drupal core's own deprecation testing results is happening. If this can't be committed quickly, that's OK but we need a planned date. Then I will regenerate the patch on that date.

OK. But just to make you aware, the automated fixing of the 610 currently only addresses the first basic layout sniff
Drupal.Commenting.Deprecated.IncorrectTextLayout

When this is committed, there will be some manual work on the 58 which could not be automated, then we can start on the other sniffs in this standard, as these are being excluded in the patch:

<rule ref="Drupal.Commenting.Deprecated">
  <exclude name="Drupal.Commenting.Deprecated.MissingExtraInfo"/>
  <exclude name="Drupal.Commenting.Deprecated.MissingSeeTag"/>
  <exclude name="Drupal.Commenting.Deprecated.VersionFormat"/>
  <exclude name="Drupal.Commenting.Deprecated.PeriodAfterSeeUrl"/>
  <exclude name="Drupal.Commenting.Deprecated.WrongSeeUrlFormat"/>
</rule>

Unpostponing now that 11 Nov is here. This is a re-roll of patch #39 which will show how many errors we have for the main layout standard, and will generate the patch to fix those that are fixable.

So patch #48 found 675 @deprecated faults on basic layout, slightly up on the 668 from 10th September.

Here is the generated patch + drupalci changes in #48. This won't fix all of the the 675 but should do a large proportion of them.

Patch #49 shows we have 58 faults which are not auto-fixed (looks like the same 58 we had in #40 back in September)
The patch alters 319 files and makes 617 corrections, so that is 91% auto-fixable. Nice that 617 + 58 = 675 which was the number of faults found in #48

This patch #50 is the actual one that can be committed (it does not include the changes to drupalci and phpcs.xml.dist) and hence will run the full test suite, not the shortcut.

Thanks @Lendude. I have just belatedly spotted how I can auto-fix another 36 of those 58 remaining failures, which will reduce the manual work even more. I can get a revised patch here today, if that is OK with you? Or do you want to go ahead now that you have marked this RTBC?

OK, this is a re-run of #48/#49/#50
Step 1 of 3 - patch #54 will check the single standard and generate the phpcs patch to fix them. This uses a new patch on the Coder issue #3057988-10: Automatically fixing @deprecated doc text

Patch #54 reported 837 coding standards messages. This time it has included the Drupal.Commenting.Deprecated.MissingSeeTag sniff even though drupalci.yml is excluding this. Nothing should have changed to cause this, but it does not really matter, it just confuses the totals

The generated patch has the expected fixes, it now alters 348 files (up 29 from 319) and makes 653 corrections (up 36 from 617)

Here is step 2 of 3 - check the generated patch running only the new sniff.

For some unknown reason the Drupal.Commenting.Deprecated.MissingSeeTag and Drupal.Commenting.Deprecated.VersionFormat sniffs are being shown as having an extra 'Deprecated' word added e.g. Drupal.Commenting.Deprecated.DeprecatedMissingSeeTag in the checkstyle.xml output in dispatcher. This should be changed by the latest Coder patch, and it worked fine yesterday in #48/#49 so I do not know why it is different today. Here's a repeat of patch #55 but with both versions of the sniff excluded. Now we should see a cleaner output (compared the the 220 messages shown in #55) of the remaining basic @deprecated standard violations that will need manual fixing.

That's better. Only 22 @deprecated basic errors remaining with this patch. 675 existing in the code - 617 - 36 = 22 so that adds up.

Step 3 of 3 - the actual patch #57 for final review, which can be committed to core 8.9.

great work on this

Thanks @Lendude

Here is the interdiff for 50-57, and that shows all the new changes are of the 8.?.? variety

Yes, those 36 extras did not have any deprecation version, so I catered for this in the corrected text by adding 8.?.?. I know these will need the actual version inserted when we come to working on the Drupal.Commenting.Deprecated.VersionFormat standard fix, but at least now the main text is correct and it only needs manual effort to find the version.

This is not fixed yet. We have done part 1 of the issue summary, committing the large automated patch. Part 2 is to fix the remaining layout problems manually. There are only 22 of these (after the 653 have been fixed automatically) as listed in comment #56. Then the final part will be to enable the sniff to ensure we do not introduce new failures.

I will try to make a these corrections, and provide a patch here, but I expect there will be details that I do not know, and will require input/review from others.

OK, I have created #3094454: Fix remaining @deprecated manually and enable the coding standard for the follow-up.

I noted that in #51 above @Lendude said that this should not go into core D9 but that the commit in #61 was at 9.0, then 8.9 and 8.8. I think this is fine, because with the standard fixed in 9.0 it means we can search and do analysis on the versions as part of the process of removing the code.

Just to let you know #3009848-5: Fully deprecate WorkflowDeleteAccessCheck is a blocker for fixing one final @deprecated tag.

Sorry to post on a closed issue, I meant to do this a few days ago.

Fixing the final 22 @deprecated coding standards and then implementing the rule has stalled due to Core tests referencing one piece of deprecated code but the deprecation warning was not being flagged - see #3009848: Fully deprecate WorkflowDeleteAccessCheck

To get this moving again, all it needs is the message to be added to Tests/Listeners/DeprecationListenerTrait to show that we are aware of its usage. This will allow the @deprecated basic layout coding standard sniff to be added to Core. The full solution to deprecating WorkflowDeleteAccessCheck can then be worked on independently. Having the standard enforced in core will ensure that we wont get into this situation again, where deprecated code usage is being masked and we are unaware of its usage.

Please can anyone who is following this issue take a look at #3009848: Fully deprecate WorkflowDeleteAccessCheck, not for the final solution but my patches in #7 and #8 which will alllow us to progress. Thanks.

Changed parent.