Fix @deprecated versions and enable Commenting.Deprecated.DeprecatedVersionFormat sniff

Hi swatichouhan012,
Thanks for looking at this, and providing the patch.

The first part of turning on the @deprecated coding standards checks is #3094454: Fix remaining @deprecated manually and enable the coding standard and this is nearly complete. When the main coder rule is enabled in Core then we can start fixing the versions. But there is no reason why this could not be committed early.

Adding parent issue

Follow more some files, left me know if it is this

Patch failed to apply

These changes in the patch should not be there

--- a/composer.json

-        "wikimedia/composer-merge-plugin": "^1.4"
+        "wikimedia/composer-merge-plugin": "^1.4",
+        "drupal/admin_toolbar": "^2.2"

and

--- a/composer.lock
+++ b/composer.lock
@@ -4,7 +4,7 @@
         "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies",
         "This file is @generated automatically"
     ],
-    "content-hash": "c6c500e6567b37ef3e100b285ab9910c",
+    "content-hash": "97a1a0e91dbaf82b8bbb3e1e300e3ad2",
     "packages": [

Not checked the rest of it.

I have updated the patch and removed non required code from the patch.

Thank sja112,
Yes you are right that this is for core 8.9. The @deprecated standard is already fixed and clean at 9.0 and 9.1.
Here is a patch which adds just the new sniff Drupal.Commenting.Deprecated.DeprecatedVersionFormat so we can see how many messages there are in total.

Argh, stale form data.

Also, I should have included a temporary change to druaplci.yml to not run the phpunit tests. We are wasting d.o. resources on this,

The results of #11 show that there are 41 version errors in total. 35 of these are 8.?.? and 6 are other problems with version formatting.

There are 25 ocurrences of 8.?.? in patch #9 so there are still 10 others not found and fixed. But this is a good start, and now we can see all 41 version errors I think this issue could be expanded to fix all of those.

I have updated the patch to include the remaining 8.?.? fixes.

Thanks sja112 however, running your patch #14 without the new sniff will not prove anything yet. It is also a waste of d.o. resource running the full set of phpunit tests when we are only at the early stages of fixing a standard. I suggest you cancel that running test, and I will then add your patch into one which runs phpcs but not phpunit. Then we can see how many you have fixed and how many are left.

@jonathan1055, I have aborted the running test.

Thanks. Here's a patch to run phpcs but not phpunit. It should finish much quicker. I also noticed that your patch has 36 fixes for 8.?.? and 6 others, making 42 errors in total.

Patch #17 still shows only 41 errors. The difference is that it does not report on core/modules/block/src/BlockInterface.php because that @deprecated tag is more fundamentally incorrect and does not match the basic text layout. We are only sniffing for incorrect versions here.

The basic layout is being addressed in #3094454: Fix remaining @deprecated manually and enable the coding standard

So here is a patch with #14 and #17 combined.

Those four fails consist of three where the version was changed from 8.?.? to 8.N.x where it should be 8.N.0
The 4th error was adding a period after the new @see url.

@sja112 do you want to fix these four last messages? If so, start with applying patch #18 so we get the fast run time, e.g. 3 minutes, instead of over 1 hour (for patch 9 and 11)

@jonathan1055 I have updated the patch to fix the remaining changes.

One more fix to go:

core/modules/file/src/Tests/FileManagedTestBase.php
line 19	The @see url 'https://www.drupal.org/node/2969361.' should not end with a period.

Updated patch.

I hope this is the last pending change.

Solving test case.

@hardik_patel_12 thanks for the patch. But all the cases were already handled in the #23. There is no need of new patch.

IMO, Customization to drupalci should be reverted at least. Working on this.

core/includes/update.inc
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 1 LINE
----------------------------------------------------------------------
 18 | WARNING | The deprecation-version 'Drupal 8.8.5' does not match
    |         | the lower-case machine-name standard: drupal:n.n.n or
    |         | project:n.x-n.n or project:n.x-n.n-version[n]
    |         | (Drupal.Commenting.Deprecated.DeprecatedVersionFormat)
 18 | WARNING | The removal-version 'Drupal' does not match the
    |         | lower-case machine-name standard: drupal:n.n.n or
    |         | project:n.x-n.n or project:n.x-n.n-version[n]
    |         | (Drupal.Commenting.Deprecated.DeprecatedVersionFormat)

One more to fix

Addressing #27 and #28

+++ b/core/modules/file/src/Tests/FileFieldTestBase.php
@@ -2,7 +2,7 @@
-@trigger_error('The ' . __NAMESPACE__ . '\FileFieldTestBase is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\file\Functional\FileFieldTestBase. See https://www.drupal.org/node/2969361.', E_USER_DEPRECATED);
+@trigger_error('The ' . __NAMESPACE__ . '\FileFieldTestBase is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\file\Functional\FileFieldTestBase. See https://www.drupal.org/node/2969361', E_USER_DEPRECATED);

@@ -14,8 +14,10 @@
- * @deprecated in drupal:8.?.? and is removed from drupal:9.0.0.
+ * @deprecated in drupal:8.5.4 and is removed from drupal:9.0.0.

Should we update the trigger_error() to 8.5.4 while we are here?

Thanks, @longwave!

Re #30, I noticed a similar one too, but i think it's a bit out of scope here.

As the sniff name indicated "Commenting.Deprecated.DeprecatedVersionFormat", the scope is limited to commenting.

in #30, it's related to namespace, and there is a function related one should be fixed if we are going to do so (see below). I doubt there are more to fix, not just the two more.

function update_fix_compatibility() {
  @trigger_error(__FUNCTION__ . '() is deprecated in Drupal 8.8.5 and will be removed before Drupal 9.0.0. There is no replacement. See https://www.drupal.org/node/3026100', E_USER_DEPRECATED);
  // Fix extension objects if the update is being done via Drush 8. In non-Drush
  // environments this will already be fixed by the UpdateKernel this point.

Let's stick with the current scope?

@sja112 the patch 20 fixed the three 8.N.x versions but introduced a change to @trigger_error removing the period when you were intending to remove the period from @see. Patch 22 did likewise altering another @trigger_error.

@hardik_patel_12 noticed this and fixed the unintended change from patch 20, but not the one in patch 22.

Customization to drupalci should be reverted

That customisation was to allow the patches to be checked in 4 minutes (just for 'validate_codebase') not 1 hours 10 mins (running full 'testing' phpunit)

longwave:

Should we update the trigger_error() to 8.5.4 while we are here?

jungle:

Let's stick with the current scope?

Yes definitely stick with the current scope. The @trigger_error standard has its own issue #3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard which is only just getting worked on at Core 9.1 (unfortunately I have not written the automatic fixing of those yet, as I was concentrating on @deprecated first).

#28 "One more to fix" The reason this is was not flagged as a version error before (in Coder 8.3.8) is because it would have been flagged as a total 'IncorrectTextLayout' failure. My enhancement in Coder 8.3.9 now has the new sniff 'MissingExtraInfo' which allows the main text to be accepted provided it is only the versions which are wrong, and hence the version formatting details can be flagged individually. Yes we may as well fix this here too.

Patch #32 removes the second unwanted @trigger_error change from #24, removes the excluding of the incorrectly named sniff Drupal.Commenting.Deprecated.PeriodAfterSeeUrl (it is called DeprecatedPeriodAfterSeeUrl and is already being catered for now in the fixes in this patch) and (temporarily) omits the testing: section in drupalci.yml to give a fast run.

Then after any final discussion (or fixing of any new standard) we can revert the drupalci.yml change and run the full hour-long test.

So #32 enables the sniff, fixes all issues, and keeps all changes tightly scoped to the exact problem at hand - therefore I would mark it RTBC but it needs the drupalci changes removing first, so back to NW for that.

Updated patch to remove drupalci changes.

Thanks longwave, yes that is exactly right.

However, these sniffs are subsidiary, the main sniff Drupal.Commenting.Deprecated.IncorrectTextLayout is being implemented on #3094454-52: Fix remaining @deprecated manually and enable the coding standard and that issue should be committed first. The change to phpcs.xml.dist will clash slightly, so after that issue is committed patch #34 will need to be re-rolled. So not RTBC yet, but hopefully the other issue will be soon.

#3094454: Fix remaining @deprecated manually and enable the coding standard is not going to be committed in core 8.9 see #92 onwards so this issue can be closed. The full Drupal.Commenting.Deprecated is enabled in 9.0 and 9.1 so there is no more to do.

Actually this is not "fixed" as there were no commits at core 8.9. I should have used "Closed (won't fix)"

Thank you to everyone who participated here, it is just a shame that we were not aware of the deadline of release-candidate for 9.0 stopping this work being committed to 8.9.