Add a rule for expected format of @deprecated and @trigger_error() text

This is a good idea. Having worked on a few of the issues in #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) there are already several variations on the deprecation comment block and text of the message. So now is a good time to make a new coder rule.

I have also noticed a few inconsistencies in the examples given in https://www.drupal.org/core/deprecation :

1. Some examples have the 'See url-link-to-change-record' in the @trigger_error message and some are missing
2. Some have "Instead use \Drupal\this\new\class" and others have "Use \Drupal\this\new\class instead"
3. Some committed changes have the full deprecated path in the @trigger_error text. If the standard is to use the __NAMESPACE__ shorthand then this should be stated explicitly.

The above are all quite minor, but we might as well get the documentation page as accurate as possible then reviewers know what they are looking for, devs can get things right in the interim, and we have a quick win before the coder change gets implemented.

I have not worked on coder sniffs but would like to learn, so I will take a look at the code and see what I can do.

I have started writing a new sniff file vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/Semantics/FunctionTriggerErrorSniff.php to verify the format of the long text message and it is working fairly well so far :-) I have now added the check to make sure that if @trigger_error is present then a PHPdoc block comment must also exist which contains a @deprecated tag.

But this is currently a "loose" relationship, because it does not check that the two relate to the same part of the code. It just checks that if one is present in the file then the other must also be. For example, an entire class which throws a runtime @trigger_error right at the top, does not fail standards if there exists a @deprecated tag in a function but it is missing the tag in the class docblock. It looks like the 'level' attribute in the $tokens array could help with this?

I've had a quick look for documentation on the output of getTokens() but not found much so I am looking at var_dump and print_r and trying to deduce what the structure is.

Jonathan
[edit: found https://github.com/squizlabs/PHP_CodeSniffer/wiki]

Here is my first attempt at writing a sniff file. It was good to do (and useful to have the discipline of writing against a different coding layout standard - phpcbf is now my friend even more than before)

For your interest I have uploaded my own two test files, one with many fails, and the other which should pass. This will obviously need proper runnable tests which I will work on next next. I have written many browser functional tests, but not done any unit tests yet (so this is also a good learning experience for me)

I wondered why the tests were not running on d.o. then noticed in the readme.md instructions that development starts on github (so probably using the Travis build and test mechanism). I have forked the repo and committed changes to branch
https://github.com/jonathan1055/coder/tree/2908391-6-deprecation-rule

I have run the tests https://travis-ci.org/jonathan1055/coder/builds/277996766

So if we mark EntityManager as @deprecated, then people know not to use it.

If we add @trigger_error() to EntityManager, it will fail all tests until all usages are removed.

So while it's a good idea to get all usages out of there, and that would be a good way to encourage that, it's also a bit of a scope and consistency problem for core development.

I see what you mean. So maybe if the file has @trigger_error() then it also has to have the @deprecated doc tag. But not the other way round, we can add @deprecated without requiring a @trigger_error(). Luckily I have not worked on that second part yet, so no effort lost. All I've done is check the format of @trigger_error() and insist that if one is found then the associated doc block also has @deprecated.

This really needs to go through the Coding Standards process. The standard for deprecation messages is not yet on
https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...
My understanding is that we don't usually add rules to Coder until they are accepting coding standards throughout the process, even if they have been adopted by the Drupal Core committers? It's a bit of a formality, but ...

Tagging for upcoming contribution days.

For what the format should be see the issue summary of #3024461: Adopt consistent deprecation format for core and contrib deprecation messages - whilst that is not yet 100% agreed it is close to the what the final standard will be.

So maybe if the file has @trigger_error() then it also has to have the @deprecated doc tag. But not the other way round, we can add @deprecated without requiring a @trigger_error(). Luckily I have not worked on that second part yet, so no effort lost. All I've done is check the format of @trigger_error() and insist that if one is found then the associated doc block also has @deprecated.

Not really. There are cases where there can only be a trigger_error(), eg. deprecated code paths. There are also cases, where there can only be a @deprecated but no trigger_error(), eg. constants being deprecated. So we cannot state that as a rule each @deprecated should have a trigger_error() or vice versa.

Also introducing trigger_error() is an ongoing process and we still want to fix the format of the existing trigger_error() and @deprecated text, so its best to have a sniff that ensures the format conforms. So checking them as-is is best.

#3024461: Adopt consistent deprecation format for core and contrib deprecation messages was posted for coding standards feedback and did not receive any further feedback other than what was already there, so looks like we can code against that now. My #17 feedback should apply, so setting to needs work.

Let's focus this issue on checking the format of the actual text pieces, and not their relation as there may be a trigger_error() without a @deprecated for deprecated code paths (an else clause for example) and there may be a @deprecated without a trigger_error() for constants for example.

1. +++ b/coder_sniffer/Drupal/Sniffs/Semantics/FunctionTriggerErrorSniff.php
   @@ -0,0 +1,147 @@
   +        // The first token must be __NAMESPACE__ not the full hard-coded path.
   +        if ($tokens[$argument['start']]['type'] !== 'T_NS_C'
   +            || $tokens[$argument['start']]['content'] !== '__NAMESPACE__'
   +        ) {
   +            $error = 'The deprecation message text must start with __NAMESPACE__';
   +            $phpcsFile->addError($error, $argument['start'], 'TriggerErrorNoNamespaceConstant');
   +        }
   +
   +        // Find the first concatenation, then ensure there are no more.
   +        if (($firstConcat = $phpcsFile->findNext(T_STRING_CONCAT, $argument['start'], $argument['end'])) > 0) {
   +            if ($phpcsFile->findNext(T_STRING_CONCAT, ($firstConcat + 1), $argument['end']) > 0) {
   +                $error = 'After the namespace, the deprecation message text should be in one string.';
   +                $phpcsFile->addWarning($error, $argument['start'], 'TriggerErrorOneString');
   +            }
   +        }
   

   @deprecated is used for things outside of namespaces, such as global constants and functions. Therefore forcing a namespace to exist does not work. The standard proposed did not cover this, but if we are to accept this then it should be allowed as either one string or a namespace plus the string.

2. +++ b/coder_sniffer/Drupal/Sniffs/Semantics/FunctionTriggerErrorSniff.php
   @@ -0,0 +1,147 @@
   +        // Check that the appropriate comment block has a tag of @deprecated .
   +        // If the @trigger_error was level 0 (entire class) then the doc comment
   +        // will also be level 0. If the @trigger_error was for a function then
   +        // the doc comment will be at one level lower.
   +        $found = 0;
   +        if ($level === 0) {
   +            $block          = $phpcsFile->findNext(T_DOC_COMMENT_OPEN_TAG, $argument['start']);
   +            $required_level = 0;
   +            $data           = array(
   +                               'class docblock',
   +                               'class',
   +                              );
   +        } else {
   +            $block          = $phpcsFile->findPrevious(T_DOC_COMMENT_OPEN_TAG, $argument['start']);
   +            $required_level = ($level - 1);
   +            $dat            = array(
   +                               'function header block',
   +                               'function',
   +                              );
   +        }
   +
   +        if ($tokens[$block]['level'] === $required_level) {
   +            foreach ($tokens[$block]['comment_tags'] as $tag) {
   +                $found += ($tokens[$tag]['content'] === '@deprecated');
   +            }
   +        }
   +
   +        if ($found === 0) {
   +            $error = 'The %s must contain a @deprecated tag if the %s is deprecated.';
   +            $phpcsFile->addError($error, $block, 'TriggerErrorNoDeprecatedTag', $data);
   +        }
   

   This should be removed.

   This is not true for deprecated code paths where trigger_error() is called but there is never a matching @deprecated. Let's not mix this into checking the actual format of messages and concentrate this issue on the format.

Adding midcamp 2019 tag

It is good to have the format agreed. And it makes it much easier to just check the two formats independently and not have rules cross-referencing occurences of @deprecated with an associated @trigger_error call, or vice-versa.

Some points to check before we re-start coding:

1. I may be able to work on this in the next few days, but equally happy if someone else wants to grab it, so unassigning myself for now.
2. Since my original patch in #6 way back in Sept 2017, Coder now has 3.x branch in addition to 2.x and I can see a major overhaul of namespaces. Both 2.x and 3.x were last updated in Sept 2018 but judging by the project front page we only need to make the change to the 3.x branch, as 2.x is unsupported.
3. Several of the last commits were from github pull requests. Is that the only way to proceed? Or is it accpetable to post patches and test files here?

@jonathan1055: for better or worse core's composer.lock even in Drupal 8.8.x-dev has this:

          "name": "drupal/coder",
            "version": "8.2.12",
            "source": {
                "type": "git",
                "url": "https://git.drupal.org/project/coder.git",
                "reference": "984c54a7b1e8f27ff1c32348df69712afd86b17f"
            },

So to me that looks like for this to be useful for core, it would need to be implemented for 8.2. I don't know the whole story / process there honestly. I do hope 8.2 and 8.3 are not THAT different though that work done on 8.2 would not be in vain if it needs to be ported to 8.3 as well.

OK, we'll do it for both branches. The work required to implement for 8.x-3.x should not be too difficult - it looks like class naming changes but the actual logic and coding of the sniff file remains identical.

Here is the updated sniff file for 8.x-2.x and two test files. I have been testing it using

phpcs --standard=Drupal -vs --colors --report-width=160 --sniffs=Drupal.Semantics.FunctionTriggerError /path/to/input/textfile

The new sniff php file should be placed in

drupalroot/vendor/drupal/coder/coder_sniffer/Drupal/Sniffs/Semantics/FunctionTriggerErrorSniff.php

and renamed to remove the _.txt

When I can run the actual unit tests for Coder I'll add these files and make a proper patch, but for now at least people can look at the work so far.

Core got updated to 8.3 of coder now, so this would be done against that. #3002206: Update the stable version of drupal/coder to ˆ8.3.1

#3024461: Adopt consistent deprecation format for core and contrib deprecation messages that is the adoption of the standard is now blocked on this coder rule.

Updated the IS to show the agreed standard layout and to be clear that the two new sniffs are independent.

My files in #23 are for the 2.x branch but can easily be made to work on the 3.x branch.

Here is a patch that add the three FunctionTriggerError files for coder 8.x-3.x Attached is the output when run against core datetime module.

Travis test builds are https://travis-ci.org/jonathan1055/coder/builds/515701140 and the new test passes (77 in total, compared to 76 in the main 8.x-3.x branch)

I will create a pull request if that is the correct way to go for development on Coder.

The pull request is https://github.com/pfrenssen/coder/pull/24

I've now written the second new sniff, for the @deprecated comment tag.
Patch has all 7 new files (3 for TriggerError, 3 for Deprecated and the good.php test)
Pull request updated https://github.com/pfrenssen/coder/pull/24
Travis tests green on https://travis-ci.org/jonathan1055/coder/builds/516914082

Great stuff, @jonathan1055! Will you be at DrupalCon this week, by any chance?

Hi webchick, no I'm afraid I wont be attending in person, I live in Birmingham, England.
I tried to contact you off-board, but could not find your contact form. Sorry for the off-topic distraction, but you did ask me a direct quesion here :-)

Haha, no worries, feel free to reach out on webchick.net's contact form if you'd like!

I patched coder and ran:

phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md --sniffs=Drupal.Commenting.Deprecated /Applications/MAMP/htdocs/8.x/core/modules/datetime

This shows the following output:

FILE: /Applications/MAMP/htdocs/8.x/core/modules/datetime/datetime.module
--------------------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
--------------------------------------------------------------------------------
 13 | ERROR | The deprecation text '@deprecated in Drupal 8.5.x and will be
    |       | removed before Drupal 9.0.x. Use
    |       | \Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface::STORAGE_TIMEZONE
    |       | instead.' does not match the standard format: @deprecated in
    |       | %in-version% and will be removed from %removal-version%.
    |       | %extra-info%.
 24 | ERROR | The deprecation text '@deprecated in Drupal 8.5.x and will be
    |       | removed before Drupal 9.0.x. Use
    |       | \Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface::DATETIME_STORAGE_FORMAT
    |       | instead.' does not match the standard format: @deprecated in
    |       | %in-version% and will be removed from %removal-version%.
    |       | %extra-info%.
 35 | ERROR | The deprecation text '@deprecated in Drupal 8.5.x and will be
    |       | removed before Drupal 9.0.x. Use
    |       | \Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface::DATE_STORAGE_FORMAT
    |       | instead.' does not match the standard format: @deprecated in
    |       | %in-version% and will be removed from %removal-version%.
    |       | %extra-info%.
 72 | ERROR | The deprecation text '@deprecated in Drupal 8.5.0 and will be
    |       | removed before Drupal 9.0.0. Use
    |       | \Drupal\Component\Datetime\DateTimePlus::setDefaultDateTime() or
    |       | \Drupal\Core\Datetime\DrupalDateTime::setDefaultDateTime()
    |       | instead.' does not match the standard format: @deprecated in
    |       | %in-version% and will be removed from %removal-version%.
    |       | %extra-info%.
--------------------------------------------------------------------------------


FILE: ...lications/MAMP/htdocs/8.x/core/modules/datetime/src/Tests/DateTestBase.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
--------------------------------------------------------------------------------
 18 | ERROR | The deprecation text '@deprecated in Drupal 8.4.0 and will be
    |       | removed before Drupal 9.0.0. Use \Drupal\Tests\BrowserTestBase
    |       | instead.' does not match the standard format: @deprecated in
    |       | %in-version% and will be removed from %removal-version%.
    |       | %extra-info%.
 18 | ERROR | Each @deprecated tag must have a @see tag immediately following
    |       | it.
--------------------------------------------------------------------------------


FILE: ...docs/8.x/core/modules/datetime/src/Tests/Views/DateTimeHandlerTestBase.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
--------------------------------------------------------------------------------
 17 | ERROR | The deprecation text '@deprecated in Drupal 8.4.0 and will be
    |       | removed before Drupal 9.0.0. Use \Drupal\Tests\BrowserTestBase.'
    |       | does not match the standard format: @deprecated in %in-version%
    |       | and will be removed from %removal-version%. %extra-info%.
 17 | ERROR | Each @deprecated tag must have a @see tag immediately following
    |       | it.
--------------------------------------------------------------------------------


FILE: ...htdocs/8.x/core/modules/datetime/src/Plugin/migrate/field/d6/DateField.php
--------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 1 LINE
--------------------------------------------------------------------------------
 23 | ERROR | The deprecation text '@deprecated in Drupal 8.4.x, to be removed
    |       | before Drupal 9.0.x. Use
    |       | \Drupal\datetime\Plugin\migrate\field\DateField instead.' does
    |       | not match the standard format: @deprecated in %in-version% and
    |       | will be removed from %removal-version%. %extra-info%.
 23 | ERROR | Each @deprecated tag must have a @see tag immediately following
    |       | it.
--------------------------------------------------------------------------------

Time: 306ms; Memory: 14MB

....which is to be expected, since none of those messages follow the standard.

I changed the lines in htdocs/8.x/core/modules/datetime/src/Plugin/migrate/field/d6/DateField.php from:

 * @deprecated in Drupal 8.4.x, to be removed before Drupal 9.0.x. Use
 * \Drupal\datetime\Plugin\migrate\field\DateField instead.

to:

 * @deprecated in drupal:8.4.0 and will be removed from drupal:9.0.0. Use
 * \Drupal\datetime\Plugin\migrate\field\DateField instead.
 *
 * @see https://www.drupal.org/node/3006470

And now get:

FILE: /Applications/MAMP/htdocs/8.x/core/modules/datetime/src/Plugin/migrate/field/d6/DateField.php
----------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 1 LINE
----------------------------------------------------------------------------------------------------------------------------
 23 | WARNING | The deprecation in-version 'drupal:8.4.0' does not match the standard: Drupal:n.n.n or Project:n.x-n.n
 23 | WARNING | The deprecation removal-version 'drupal:9.0.0' does not match the standard: Drupal:n.n.n or Project:n.x-n.n
----------------------------------------------------------------------------------------------------------------------------

This seems to be in conflict with the recommendation from the standard's issue summary, which states:

@deprecated docblock format

@deprecated in %in-version% and will be removed from %removal-version%. %extra-info%.
@see  %cr-link%

For example:

 * @deprecated in drupal:8.7.0 and will be removed from drupal:9.0.0. Use
 *   date('c', $date) instead.
 *
 * @see https://www.drupal.org/node/2999991

(In other words, the standard recommends the machine-readable project name vs. the human-readable name.)

So I believe this needs work for that, but is hopefully a quick-ish change.

Next up, ran the sniff for the trigger_error() stuff:

phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md --sniffs=Drupal.Semantics.FunctionTriggerError /Applications/MAMP/htdocs/8.x/core/modules/datetime

Output:

FILE: /Applications/MAMP/htdocs/8.x/core/modules/datetime/datetime.module
-----------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------------------------
 79 | WARNING | The deprecation version 'Drupal 8.5.0 and will be removed before Drupal 9.0.0' does not match the standard: Drupal:n.n.n or
    |         | Project:n.x-n.n
 79 | WARNING | The change-record url 'https://www.drupal.org/node/2880931.' does not match the standard: http(s)://www.drupal.org/node/n
-----------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /Applications/MAMP/htdocs/8.x/core/modules/datetime/src/Tests/DateTestBase.php
-----------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------------------------
 5 | WARNING | The deprecation version 'Drupal 8.4.0 and will be removed before Drupal 9.0.0' does not match the standard: Drupal:n.n.n or Project:n.x-n.n
 5 | WARNING | The change-record url 'https://www.drupal.org/node/2780063.' does not match the standard: http(s)://www.drupal.org/node/n
-----------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /Applications/MAMP/htdocs/8.x/core/modules/datetime/src/Tests/Views/DateTimeHandlerTestBase.php
-----------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------------------------
 5 | ERROR | The deprecation message '\Drupal\datetime\Tests\Views\DateTimeHandlerTestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal
   |       | 9.0.0. Instead, use \Drupal\Tests\BrowserTestBase' does not match the standard format: %thing% is deprecated in %in-version%. %extra-info%.
   |       | See %cr-link%
-----------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: /Applications/MAMP/htdocs/8.x/core/modules/datetime/src/Plugin/migrate/field/d6/DateField.php
-----------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-----------------------------------------------------------------------------------------------------------------------------------------------------------
 34 | ERROR | The deprecation message 'DateField is deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.x. Use
    |       | \Drupal\datetime\Plugin\migrate\field\DateField instead.' does not match the standard format: %thing% is deprecated in %in-version%.
    |       | %extra-info%. See %cr-link%
-----------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 314ms; Memory: 14MB

Changed the line in file src/Tests/DateTestBase.php from:

@trigger_error('\Drupal\datetime\Tests\DateTestBase is deprecated in drupal:8.4.0 and will be removed from drupal:9.0.0. Use \Drupal\Tests\BrowserTestBase instead. See https://www.drupal.org/node/2780063.', E_USER_DEPRECATED);

to:

@trigger_error('\Drupal\datetime\Tests\DateTestBase is deprecated in drupal:8.4.0. It will be removed from drupal:9.0.0. Use \Drupal\Tests\BrowserTestBase instead. See https://www.drupal.org/node/2780063', E_USER_DEPRECATED);

(Removing the period from the URL; took me a bit to figure out what that second error meant.)

This gives the same (erroneous) error:

FILE: /Applications/MAMP/htdocs/8.x/core/modules/datetime/src/Tests/DateTestBase.php
-------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------
 5 | WARNING | The deprecation version 'drupal:8.4.0' does not match the standard: Drupal:n.n.n or Project:n.x-n.n
-------------------------------------------------------------------------------------------------------------------

...but everything else looks good!

Thanks for the review.

1. Good point about the machine-name vs readable. Definitely the agreed standard was for one parsable string, and we have the ':' there to enforce that. So, it just comes down to the upper or lower-case first letter. I presume that the proper machine name is all lower-case, and if so I can very easily fix the sniff regex.
2. I have just noticed in the standards issue examples that one of the urls is https://www.drupal.org/project/paragraphs/issues/2911242 but currently I have only allowed /node/n in the url. So that should be made more flexible to allow /project/aaa/issues/n
3. Yes, when I was testing my code it also took me a few looks to realise that a final period after the url was the cause of a problem. I can easily add a new hint for that specific fault, as we want to make it as easy as possible for devs to fix the code

Here's a patch which addresses the three points in #35.
The interdiff is just for the sniff.php files but I can do one for the test files if needed.
Tests pass here https://travis-ci.org/jonathan1055/coder/builds/518434536

I have just noticed a small error. In the change for triggerError version I forced the 'project' to be lower-case as requred, using [a-z\d_] in the patern. But I forgot to do this in the Deprecated tag snif.

New full patch attached, and interdiff from 36 to 37.

I did a quick review of the most recent patch and wanted to say "thanks" for the great work here.

I noticed a few things I wanted to inquire about:

In coder_sniffer/Drupal/Test/Semantics/FunctionTriggerErrorUnitTest.inc

+// No second parameter, so cannot fail it.
+@trigger_error('CommentTestBase is deprecated in drupal 8.4.0');
+// Not E_USER_DEPRECATED, so cannot fail it.
+@trigger_error('CommentTestBase is deprecated in drupal 8.4.0', E_USER_SOMETHING_ELSE);

Should that respect the same standard as previously noted? e.g.

drupal:8.4.0

and not

drupal 8.4.0

In coder_sniffer/Drupal/Test/Semantics/FunctionTriggerErrorUnitTest.inc

+@trigger_error('drupal_set_message() is deprecated Drupal:8.5.0. It will be removed from Drupal 9.0.0. Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead. See https://www.drupal.org/node/2774931', E_USER_DEPRECATED);
+// Wrong first part - bad spacing.
+@trigger_error('drupal_set_message() is   deprecated in Drupal:8.5.0. It will be removed from Drupal 9.0.0. Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead. See https://www.drupal.org/node/2774931', E_USER_DEPRECATED);
+// Wrong first part - no final dot.
+@trigger_error('drupal_set_message() is deprecated in Drupal:8.5.0 Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead. See https://www.drupal.org/node/2774931', E_USER_DEPRECATED);
+// Wrong first part - nothing before 'is deprecated'.
+@trigger_error(__NAMESPACE__ . ' is deprecated in Drupal:8.5.0. It will be removed from Drupal 9.0.0. Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead. See https://www.drupal.org/node/2774931', E_USER_DEPRECATED);
+// Missing 'extra info' part.
+@trigger_error('drupal_set_message() is deprecated in Drupal:8.5.0. See https://www.drupal.org/node/2774931', E_User_Deprecated);
+// String would be OK but is in more than one part.
+@trigger_error('drupal_set_message() is deprecated in Drupal:8.5.0. ' . 'It will be removed from Drupal 9.0.0. Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead. See https://www.drupal.org/node/2774931', E_USER_DEPRECATED);

Should this be

Drupal:9.0.0

not

Drupal 9.0.0

.

I am also unclear on casing. It's my understanding most project references should be lower cased, not upper. This may need feedback, but instances of

Drupal:....

may need to be

drupal:....

. Also see

Project:8.6-2.x

as another example.

Hi nerdstein, Thanks for the review, and I'm pleased you appreciate the work. To answer your questions:

1. In FunctionTriggerErrorUnitTest.inc:

// No second parameter, so cannot fail it.
@trigger_error('CommentTestBase is deprecated in drupal 8.4.0');
// Not E_USER_DEPRECATED, so cannot fail it.
@trigger_error('CommentTestBase is deprecated in drupal 8.4.0', E_USER_SOMETHING_ELSE);

Those lines cannot be failed because they do not have the correct E_USER_DEPRECATED second parameter. So I specifically left the text as non-conforming, to prove that those lines do not fail.

2. In your second question on FunctionTriggerErrorUnitTest.inc, those exampes of Drupal 9.0.0 are part of the %extra-info% section of the standard, which is free text. Maybe it is confusing to use an example which has that in, but as it is free text there is no standards check. Those examples where written when the standard was going to have "and removed from ..." in the trigger error text, but that got dropped. It does remain as part of the @deprecated standard text though.

3. The casing of drupal: and project: should all be lower case, and this is enforced where they form part of %in-version% or %removal-version% but when this is in free-text it is not checked. See the test line 52:

// Wrong case for Project.
@trigger_error(__NAMESPACE__ . '\CommentTestBase is deprecated in Project:8.6-2.x. Use \Drupal\Tests\comment\Functional\Views\CommentTestBase instead. See http://www.drupal.org/node/2908490', E_USER_DEPRECATED);

This will correctly give 1 warning as expected when the phpunit test is run. The tests are not run on d.o. infrastucture, but you can see my results on Travis & Github https://travis-ci.org/jonathan1055/coder/branches - look for the 2908391-deprecation branch.

I have put the issue back to 'Needs Review' as from the three points above I don't think anything needs to be changed in the code, but I'm happy to make changes if they are required.

Jonathan

I reviewed the pull request and I think we are good to go here!

Committed, thanks!

That's great news.

Thanks also for releasing coder:8.x-3.2 so that the new rule becomes effective right away.

I have raised #3048244: Update the stable version of drupal/coder to ˆ8.3.2 to allow work on fixing core to adhere to the new standards.

While working on #3049887: New phpcs failures under drupal/coder 8.3.3, I realized there was an issue with the regexp for matching the project version that was committed... (It's missing a quantifier).
The following deprecation:

 * @deprecated in commerce:8.x-2.7 and will be removed from commerce:9.x-3.0.
 * Use CurrencyFormatterInterface instead.
 * @see https://www.drupal.org/node/2971732</blockquote>

isn't considered as "valid":

-------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------------
 8 | WARNING | The deprecation in-version 'commerce:8.x-2.0' does not match the standard: drupal:n.n.n or project:n.x-n.n
 8 | WARNING | The deprecation removal-version 'commerce:9.x-3.0' does not match the standard: drupal:n.n.n or project:n.x-n.n
-------------------------------------------------------------------------------------------------------------------------------

With the attached patch, those warnings are gone.

Thanks! Please open a new issue to track this.

Please also add a test case and create a pull request against https://github.com/pfrenssen/coder

The follow-up issue is #3050166: Contrib project version is not correctly matched in @deprecated tag

Due to the discussion from #89 onwards on #3024461-89: Adopt consistent deprecation format for core and contrib deprecation messages I have set this issue back to 'needs work' because the agreed standard is no longer agreed and the sniffs will need some adjustment.

Instead of opening a new issue I thought it best to stick with this one, as there are 22 followers here, and the more people who are watching this, the more chance of a someone doing a review and we can get it sorted. However, if @klausi or @pfrenssen want this one closed and a new issue started, that's OK with me, just say.

Fine with me if you want to continue here. Please update the issue summary once you start implementing again with examples or best guess of what the standard will look like at this point.

Here is the new https://github.com/pfrenssen/coder/pull/40

I have updated the IS with the likely new standard, and work can still progress on the "associated @deprecated tag" part, even if the coding standards wording is not finalised.

Updated IS to further explain how the trigger_error format is affected by the presence of a @deprecated tag.

Thanks for updating the issue summary @jonathan1055 - note I don't think we can automate the discovery of associate @trigger_error and @deprecated. But even if the effort to do the associated standard in a PHPCS rule proves impossible I still think you right to talk about a strict version in the presence of an associated @deprecated.

Yes @alexpott you are right. The strict version should be the standard when there is an associated @deprecated tag, but the rule will not enforce the same actual text, only enforce the standard format. The default is the relaxed version, and only when a definitive associated @deprecated tag is found should the strict format be checked. The attached patch mirrors the PR for those who want to try it here for themselves. They key new bit is:

// Check if there is a @deprecated tag in an associated doc comment
// block. If the @trigger_error was level 0 (entire class or file) then
// try to find a doc comment after the trigger_error also at level 0.
// If the @trigger_error was at level > 0 it means it is inside a
// function so search backwards for the function comment block, which
// will be at one level lower.
$strictStandard    = false;
$triggerErrorLevel = $tokens[$stackPtr]['level'];
if ($triggerErrorLevel === 0) {
    $requiredLevel = 0;
    $block         = $phpcsFile->findNext(T_DOC_COMMENT_OPEN_TAG, $argument['start']);
} else {
    $requiredLevel = ($triggerErrorLevel - 1);
    $block         = $phpcsFile->findPrevious(T_DOC_COMMENT_OPEN_TAG, $argument['start']);
}

if (isset($block) === true && $tokens[$block]['level'] === $requiredLevel && isset($tokens[$block]['comment_tags']) === true) {
    foreach ($tokens[$block]['comment_tags'] as $tag) {
        $strictStandard = $strictStandard || (strtolower($tokens[$tag]['content']) === '@deprecated');
    }
}

In my testing this has produced the necessary outcomes, but I have not run it on the entire core codebase. If we miss some obscure cases and allow the relaxed format, how serious would that really be in practice? The relaxed format still has both versions (so we can still parse that data) and the developer would still be allowed to write the strict version of the text, which was the problem with the current standard as implemented in the new sniffs.

The calls to $phpcsFile->findNext and $phpcsFile->findPrevious could be tightened by using an 'end' parameter to limit the scope of searching, or the discovered $block could be further checked to make sure it is adjacent to the current location, or whatever, but first we need to find the cases where the above logic gives the wrong answer. Then we can improve it.

The pull request looks good to me!

Before we commit this please do a manual test run on Drupal core if it flags the correct things. Does not have to be a thorough test on all cases, more like a smoke test to double check if this works as intended. I think it is fine to do further fine tuning in follow-up issues (or here) if we miss something after this.

Next steps:
1) Manual test Drupal core with this change
2) Commit this change to Coder
3) Start working on core issue to fix all the messages. This can be done by updating Coder to the dev version in core. That way you can check on the testbot if your message fixes are accepted by Coder.
4) Create a new Coder release if we don't find problems while working on the core issue.

Thanks Klausi for the review, and for indicating that what I've coded might be OK. I agree with your next steps. The standard can be agreed but determining whether to use the strict or relaxed version might not be able to be made perfect first time so there will (probably) be adjustments after step (3) has been worked on.

We might need to add another step - to get final agreement on the changed standard text, from key personnel involved in #3024461-88: Adopt consistent deprecation format for core and contrib deprecation messages such as @gabor, @alexpott, @xjm, @pfrenssen. Would be nice to have this sooner rather than later.

@jonathan1055 I'm concerned that

The default is the relaxed version, and only when a definitive associated @deprecated tag is found should the strict format be checked.

might prove tricky in practice where a code block has multiple trigger_error's and a single @deprecated. I'm not 100% that this happens in practice but I can imagine it.

Thanks @alexpott.

a code block has multiple trigger_error's and a single @deprecated. I'm not 100% that this happens in practice but I can imagine it.

The relaxed version can still be written in the strict format if the developer knows and wants that. This was the whole point of re-opening and changing the standard. I do not think it matters if a few that should be strict are relaxed, because one the main points of the standard is to allow parsing out of the %deprecation-version% and %removal-version% which can be done whichever format is used.

When you say "code block" with multiple trigger_error calls, if that code block is a function then it wont have a @deprecated tag because the multiple calls to trigger_error by definition will mean that some varieties of actions within the function are deprecated, such as not using a parameter, or the wrong type of interface. All of these should use the relaxed version. If the code block is a class whcih is deprecated then there will only be one trigger_error call at that level. At least, that is what I am expecting.

Klausi's suggestion of getting this runable on d.o. testbot so that we can discover the problem cases is exactly the next thing we need to do, then we can see how many times the logic does not work.

Committed that pull request, thanks a lot Jonathan!

Thanks Klausi. Just for reference here is the commit

Refering back to your comments in #54 we've done tasks (1) and (2), so now need to look at:

(3) Start working on core issue to fix all the messages. This can be done by updating Coder to the dev version in core.

Can that be achieved by patching core's composer.json or drupalci.yml? i.e. the change does not need to be commited to core first, but can be included in the patch which is being tested.

@jonathan1055 we need a new release of coder or we can file a patch that pins coder to the commit in composer.json, changes phpcs.xml and tries to make the changes - at least if we do that we can start to see the scope - which is likely to be massive.

@alexpott in #3048495-22: Fix Drupal.Semantics.FunctionTriggerError coding standard I made a patch which attempted to use coder 3.x-dev but it was not successful, PHPCS still ran coder 8.3.2.

Is the drupalci.yml syntax + coder-version: ^8.3@dev valid? I have seen this in another project but I don't think drupalci.yml will do anything with the coder version. The composer.json change was also ineffective.

But at least we do know the scope of the problem in core now - 846 trigger_error messages as at 8th May and 948 @deprecated messages as at 26 April.

Here is a patch, not for committing (or for testing) but added to allow drupal.org testing to use the updated sniff code. This patch applies to Coder 8.3.2 and will be used in a patched drupalci.yml to test the new code on #3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard

I released Coder 8.3.4, so the patch should not be needed. Just upgrade Coder in Drupal core.

Thanks for the release 8.3.4.

Just upgrade Coder in Drupal core.

I tried that already by patching drupal's core/composer.json to require 8.3.3 (when that was the latest release) but it made no difference. The patch was applied OK but the test environment dependencies where not re-evaluated during the build phase. I also tried changing the root-level composer.json file, and still the dependencies rebuild was not triggered like it is when patching contrib composer.json.

I know it is only a temporary solution, but the patch I added in #63 above did work as expected, it was applied via a change to drupalci.yml and the newly updated sniff code was used, and we got the new standards output in https://www.drupal.org/pift-ci-job/1303073

Ah ok, not sure what went wrong in your case, the upgrade in #3041983: Fix unused imports and update Coder to 8.3.4 seems to be working normally.

I was only updating composer.json, which is sufficient in contrib to trigger a dependencies re-evaluation. However, that was in a project which does not have a composer.lock. So I think that may have been the difference.

Given that #3041983: Fix unused imports and update Coder to 8.3.4 may not be done very soon, I've expanded the patch from #63 to update both the sniffs, so that I can use it on #3048498: [≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard too.

Thanks @klausi, @webchick, @Gábor, @alexpott, @xjm for you help and review. Unassigning as I think this issue is done now. If there are further tweaks to the coder sniffs we can start a new issue.