Adopt consistent deprecation format for core and contrib deprecation messages

Adopting this standard formally would help the API module parse deprecation messages, to allow filtering on pages like
https://api.drupal.org/api/drupal/deprecated/8.6.x
so that we could filter to stuff slated to be removed in D9 vs. D10, for instance. This would be a good thing.

Also, Core has already adopted this standard.

Also, it is in line with the standard we currently have, which says what information needs to be provided, but doesn't provide a standard wording -- having a standard wording makes it easier for both humans and machines (such as the API module) to read.

All in all -- +1 from me.

I fully support and endorse standardising the format of @deprecation annotations in the coding standards. Clearly our standards originate from the Symfony deprecation rules but that doesn’t explicitly formalize the message or annotation, just the use of annotations and trigger_error. Even our core deprecation documentation simply gives an example, rather than formalizing it. I would like to see explicitly something documented like:

@deprecated in Drupal <major>.<minor>.<patch> and will be removed before Drupal <major>.<minor>.<patch>. Use <replacement> instead. 
@see <change record URL>

in the coding standards so that contrib could make use of it as well.

Is <project> supposed to be the project short name here or the Human Readable Name? Because if it contains spaces, that makes it more difficult to parse out <project> <version>, I think?

EDIT: put things that looked like HTML codes in code blocks so you can see them. Oops.

++ That'd certainly be cleaner feeling than having the same boiler plate text many many many times.

I don't understand comment #7? I think we're talking about making the same boiler-plate text appear many times...

Maybe #7 was referring to #3024555: Create a static Utility method wrapper for generating deprecation messages for @trigger_error calls

Possibly, but this issue needs to keep on track for what it is about: the coding standards for the @deprecation tag message that goes into doc blocks.

+1 to this proposal. I think <project> should be the shortname. Also the think the version should be separated from the project with a colon for example
drupal:8.7.x or paragraphs:8.x-3.x

The colon is inspired by how you required a specific version in composer - ie. $ composer require refinery29/test-util:0.10.2

For final discussion, this should really have an actual proposal in the IS. Do we go with #5? And is <project> the short name, to make parsing easier? Do we want Composer syntax as per #12?

But all in all, I’m +1 on this – and would defer to jhodgdon’s opinion on specifics, as it should be as reliable to parse as possible.
Since colon vs. space shouldn’t matter, I’d vote for a space, as that’s how Core is already doing it, and is better to read for humans.

@drunken monkey what do mean

as that’s how Core is already doing it,

- it the messages already? We're not using shortnames so the point is a bit moot. Also the only place version info is listed in a string like this are composer files or .info.yml files. The more future thinking solution is using the composer notation.

There's another problem. Using the docblock annotations is not really that great. There can be multiple deprecations in the a code block - ie. different arguments deprecated at different times. The thing we need a standard format for is the message in @trigger_error because these are the things we have to handle in the deprecation listener - not the doc block. Atm in the the current standard is something like:

@trigger_error('drupal_move_uploaded_file() is deprecated in Drupal 8.0.x-dev and will be removed before Drupal 9.0.0. Use \Drupal\Core\File\FileSystemInterface::moveUploadedFile(). See https://www.drupal.org/node/2418133.', E_USER_DEPRECATED);
 

So I think we can agree that this message should be split into three parts:

• project & version info
• what to do
• change record

So I proposed that this becomes:

@trigger_error('drupal_move_uploaded_file() is deprecated in drupal:8.0.x-dev and will be removed before drupal:9.0.0. Use \Drupal\Core\File\FileSystemInterface::moveUploadedFile(). See https://www.drupal.org/node/2418133', E_USER_DEPRECATED);

So the format is (for core): %thing% is deprecated in project:major.minor.patch-modifier and will be removed before project:major.minor.patch-modifier. %what-to-do%. See %CR link%

So the format is (for contrib): %thing% is deprecated in project:drupal-major.x-minor.patch-modifier and will be removed before project:drupal-major.x-minor.patch-modifier. %what-to-do%. See %CR link%

Points to note:

• %thing% can be a class name / method / function / constant / interface name / service / hook and maybe other things - we have to check they can all be followed by is
• %what-to-do%. can be anything - the format of Use blah instead. is not always possible.
• I think we should consider allowing and will be removed before project:major.minor.patch-modifier to be omitted when it is the next major version. Less boilerplate and I think if something is deprecated in 8.7.x then the fact it will not be there in 9.0.0 is not surprising.

Here are examples from the standard:
@trigger_error('The concept of container scopes is deprecated since version 8.0.0 and will be removed in 9.0.0. Omit the third parameter. See https://www.drupal.org/node/2549395.', E_USER_DEPRECATED);

 @trigger_error('SafeMarkup::isSafe() is deprecated in Drupal 8.0.0 and will be removed before Drupal 9.0.0. Instead, you should just check if a variable is an instance of \Drupal\Component\Render\MarkupInterface. See https://www.drupal.org/node/2549395.', E_USER_DEPRECATED);

    deprecated: The "%service_id%" service is deprecated. You should use the 'router.no_access_checks' service instead. See https://www.drupal.org/node/2317841.

The standard was not written with machine parsing these messages in mind. We have an aim to allow machines to parse these messages. So in order to achieve the aims of this issue we need to fix the core standard before moving it to the contrib standard - because we don't want it to change much once it is there.

As the issue summary says

All of these require know-how about the deprecation version and the removal version, which is only retrievable from the code if a standard deprecation format is used.

Is it possible to have a dedicated function call instead of the generic trigger_error(), something like trigger_deprecation_error()?

The reason I ask is that if this was adopted, then on api.drupal.org you could look at the function calls to trigger_deprecation_error() rather than trying to parse the @deprecation tag. Or api.drupal.org could even parse out the arguments to trigger_deprecation_error() and save them somewhere -- we are already parsing out various function calls (such as to theme()) and saving parts of them, and during parsing, we have information about function arguments. But I think trigger_error() is being used for other things... or is it?

Oh sorry, looks like that is the proposal in #15?

re #17 we can't have a method trigger the error. Because that would change where the deprecation is triggered from and the call chain is important. But we can have a method that creates the message in whatever format we agree for the @trigger_error call.

W.r.t to api.d.o parsing the annotations - one problem is the annotation allows for one deprecation message whereas there can be multiple deprecations per block of code. And different things deprecated at different times. But api.d.o could look for @trigger_error('the message', E_USER_DEPRECATION); because that is how all deprecations are triggered.

The problem with looking for @trigger_error instead of @deprecated in the API module is that we probably have contrib modules and/or D7 and/or ??? that is using @deprecated without trigger_error. Switching to only parsing on trigger_error would probably mean we miss some?

@jhodgdon not only contrib - there is tonnes of @deprecated without @trigger_error in core. I think api.d.o might need to scan both :(

This issue title confused me, because I said "We've got one already". Trying to make it clearer that we're looking to improve our existing core standard to a standard that both core and contrib can use.

Just a note that we can't always say Use Object::method() instead. for the replacements. E.g. a recent Layout Builder fix introduced a deprecation that said:

The section list should be derived from context.

...because there was no 1:1 replacement for the thing being deprecated; you had to change your implementation beyond that in order to get the same result. The CR (linked immediately afterward) explained how.

For any standard we pick here, we should write a quick script to scan core and find examples of non-compliant deprecation messages, to validate the standard and find any usecases we're missing.

We also have deprecation messages for new constructor arguments of the form

The entity_display.repository service must be passed to EntityViewBuilder::__construct(), it is required before Drupal 9.0.0.

i.e no mention of 'and will be removed by'

@larowlan brought up the issue of constructor additions in slack and the fact this format does not deal with them. I think actually this format does not generally deal with additions so we should have a format for them too. Something like:

%thing% is added in %added-version% and is required before %required-version%. %what-to-do%. See %cr-link%

So for example:

@trigger_error('The $entity_repository argument is added in drupal:8.7.0 and is required before drupal:9.0.0. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

I think %what-to-do% can be omitted in this instance.

[Edit: thanks @jhodgdon]

Good point, and good catch @larowlan!

Just pointing out that the format suggested in #31 still says "and will be removed" and doesn't match the example. Probably copy/pasted and then not modified from the original format?

Noticed an inconsistency in the summary examples too.

Updating issue summary for addition standard.

@Gábor Hojtsy pointed out in slack that:

• Is 0.x a valid contrib branch? For "paragraphs:9.x-0.0"
• Also the E_User_deprecated looks odd at first for additions but what is depreacted there is the argument being optional, so I guess it is fine.

I think 0.x is not a valid branch at least I don't think you can set that up on d.o and in semver 0.x.y has a special meaning so I've changed the example.

Yeah E_USER_DEPRECATION is odd for additions but it feels the best way we have to do this. In effect we are adding a deprecated code path with the addition - i.e. calling the method with a null.

Thinking about this, could we handle this case with the deprecation format? Ie. instead the format could be:
@trigger_error('Calling __construct() without the $entity_repository argument is deprecated in drupal:8.7.0. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

Can we just add some flexibility in the verbs? In terms of Regex, something like:

/(.*) is (deprecated|supported) in ([^:\s])+:([0-9]+\.[0-9]+\.[0-9]+) and will be (removed|required) before ([^:\s])+:([0-9]+\.0\.0)\. (.*). See (https?:\/\/.*)\./U

So @trigger_error('Calling __construct() without the $entity_repository argument is deprecated in drupal:8.7.0. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

becomes

@trigger_error('Calling Foo::__construct() with the $entity_repository argument is supported in drupal:8.7.0 and will be required before drupal:9.0.0. Pass an implementation of /Drupal/Core/Entity/EntityRepositoryInterface as the xth argument in the Foo Constructor. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

@mikelutz I like the idea.

Pass an implementation of /Drupal/Core/Entity/EntityRepositoryInterface as the xth argument in the Foo Constructor for me this repeats the first bit - ie Calling Foo::__construct() with the $entity_repository argument and unnecessarily verbose.

Well, then maybe the %what to do% block is optional at committer discretion if the %thing% being deprecated/added makes %what to do% self-evident. I'm not generally opposed to verbosity in documentation, but I don't like redundancy. EntityManager stuff adds an extra layer of trickery in that it references a generic CR. Most deprecations have a dedicated CR where things can be made very clear for that explicit piece of code.

@mikelutz the one thing that gives me a bit of pause is that the @deprecated should NOT be included for additions. So we need to extend the standard to say something like:

• If the verb is deprecated the @deprecated docblock is required.
• If the verb is supported the @deprecated docblock must be omitted.

@mikelutz also note (^[:\s])+:([0-9]+\.[0-9]+\.[0-9]+) only copes with the core project - it doesn't cope with contrib - ie paragraphs:8.x-1.0

Updated the issue summary with the latest change to the proposal from @mikelutz

Starting to use this for constructor additions in #3023981: Add @trigger_error() to deprecated EntityManager->EntityRepository methods, hoping it won't change too much anymore :)

This is a case that fits neither additional nor removal:

  /**
   * Returns the entity repository.
   *
   * @return \Drupal\Core\Entity\EntityRepositoryInterface
   *   The entity repository.
   */
  protected function getEntityRepository() {
    @trigger_error('Classes that use EntityTranslationRenderTrait must provide a getEntityRepository() method since drupal:8.7.0. This implementation will become abstract before drupal:9.0.0. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
    return \Drupal::service('entity.repository');
  }

I used "must provide .. since drupal:8.7.0". Something like that might also be an option for the addition format, "is supported" sounds too weak/optional to me, but no strong feelings on that..

Fixed a typo in IS introduced in #33 when "prior to" was changed to "before to"

How about:

/(.*) (is deprecated|is supported|should (.+)) in ([^:\s])+:([0-9]+\.[0-9]+\.[0-9]+|[0-9]+\.x-[0-9]+\.[0-9]+(-(alpha|beta)[0-9]+)?) and (will be removed|will be required|must do so) before ([^:\s])+:([0-9]+\.0\.0|[0-9]+\.x-[0-9]+\.[0-9]+)\.((.*).)? See (https?:\/\/.*)\./U

That would support the following, according to online regex testers..

@trigger_error('Classes that use EntityTranslationRenderTrait should provide an getEntityRepository() method in drupal:8.7.0 and must do so before drupal:9.0.0. This implementation will be abstract in Drupal 9. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
@trigger_error('Calling Foo::__construct() with the $entity_repository argument is supported in drupal:8.7.0 and will be required before drupal:9.0.0. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);
@trigger_error('Foo is deprecated in paragraphs:8.x-2.3-beta4 and will be removed before paragraphs:8.x-2.4. See https://drupal.org/node/9999999.', E_USER_DEPRECATED);

I don't quite understand the intent of the use of "supported" instead of "deprecated" in these examples above. It seems confusing and magical. I don't get the binary.

Instead of :

@trigger_error('Calling Foo::__construct() with the $entity_repository argument is supported in drupal:8.7.0 and will be required before drupal:9.0.0. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

I would have said:

@trigger_error('Calling Foo::__construct() without the $entity_repository argument is deprecated in drupal:8.7.0. The $entity_repository argument will be required before drupal:9.0.0. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

I don't think we should be too strict with the regex. We want to throw a coding standards error if they omit required info (like the version, what's deprecated, the change record, what to do instead). I really think we should loosen it up to the template to allow for human words that explain things:

%deprecated-thing% is deprecated in %project:%version and will no longer be supported in %project:%version. %thing_to_do_instead% See: %change_record%.

Also, it seems like maybe this "supported" stuff should be split off into its own discussion?

+1 for the better message alternative in #49, it is positive and clear. Having "is supported in ... and will be required by ..." is unnecessarily confusing. By definition, when the deprecated message is seen by anyone at a specific release then the alternative will already be available at that release. So the message is redundant, and we can use the better and consistent format as suggetsed in the second code block of #49.

However, the template suggested in quotes at the end is not necessarily the best though: "will no longer be supported in" ? What was wrong with the original "will be removed before". If the thing is being removed then that is very clear that something else has to be used. "no longer supported" could be interpreted as "but use it at your own risk" as is the case with other non-supported things in software.

So the bits of information we need to extract from the this string are:

• The project the deprecation is from
• The version the deprecation (or addition or change) occurred
• The version the deprecation will no longer be triggered and the deprecated code path will be removed

We need this information so that modules can say I'm update to with drupal:8.5.0 and paragraphs:8.x-2.0 and then the test system can behave as expected
We need the version the deprecation will be removed so we can build tools to remove deprecations automatically.

The rest of the standard being proposed here is for humans so that the messages are easy to scan for the important information.

I'm +1 on using the deprecation language for additions as detailed in #49 - I proposed something similar in #38. However the latest proposal for additions splits up the %in-version% and %before-version% which makes parsing harder and we need both.

Perhaps we can use more generic language to get around this.

@trigger_error()

%thing% is deprecated in %in-version% and will not be supported in %before-version%. %what-to-do%. See %cr-link%

Deprecation example:

@trigger_error("addDescription configuration key is deprecated in paragraphs:8.x-1.0 and will not be supported in paragraphs:9.x-1.0.  Use add_description instead. See https://www.drupal.org/project/paragraphs/issues/2911242", E_USER_DEPRECATED);      

Addition example:

@trigger_error('Calling Foo::__construct() without the $entity_repository argument is deprecated in drupal:8.7.0 and will not be supported in drupal:9.0.0. Provide the $entity_repository argument. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

Berdir's example from #46

@trigger_error('Using EntityTranslationRenderTrait without implementing a getEntityRepository() method is deprecated in drupal:8.7.0 and will not be supported in drupal:9.0.0. Implement ::getEntityRepository() on the class. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);

There is one point from #51 that is kind of important though: "will not be supported" isn't really strong enough to get across the point of "your code will not work any more because the ability to do this is going away". The examples in #52 all use the "will not be supported" phrase. Can we think of something else? Maybe "will not work" or "will break" or ... I don't really like either of those, but "support" is weak.

So I'm a bit dubious about the need to parse the version that something is removed in. I think the automatic removal of deprecations is a bit of a pipe dream. So maybe thinking that way allows is to change things all around and make parsers life easier.

@trigger_error()
%thing% is deprecated in %in-version%. %extra-info%. See %cr-link%

Deprecation example:

@trigger_error("addDescription configuration key is deprecated in paragraphs:8.x-1.0. It will not be supported in paragraphs:9.x-1.0. Use add_description instead. See https://www.drupal.org/project/paragraphs/issues/2911242", E_USER_DEPRECATED);

Addition example:

@trigger_error('Calling Foo::__construct() without the $entity_repository argument is deprecated in drupal:8.7.0. The $entity_repository argument will be required in drupal:9.0.0. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

Berdir's example from #46

@trigger_error('Using EntityTranslationRenderTrait without implementing a getEntityRepository() method is deprecated in drupal:8.7.0. EntityTranslationRenderTrait::getEntityRepository() will be made abstract in drupal:9.0.0. Implement ::getEntityRepository() on the class. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);

Being able to tell apart up to which version is your contrib compatible is only possible if we know the fails you are experiencing are not yet relevant. What are other means we can achieve the same if not information embedded in the messages?

For this the key bit of information is when the deprecation occurred. I think if we do deprecated things in Drupal 8 for Drupal 10 then the deprecation message should be something like

@trigger_error('Calling Foo::__construct() without the $entity_repository argument is deprecated in drupal:9.0.0. The $entity_repository argument will be required in drupal:10.0.0. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

I.e. it is a future deprecation because that reflects the reality.

And similarly for contrib pargraphs could do this in paragraphs 8.x if it wanted too...

@trigger_error("addDescription configuration key is deprecated in paragraphs:9.x-1.0. It will not be supported in paragraphs:10.x-1.0. Use add_description instead. See https://www.drupal.org/project/paragraphs/issues/2911242", E_USER_DEPRECATED);

So your contrib module could say I'm up to date with drupal:8.9.0 and paragraphs:8.x-2.0 and then these messages will not trigger a warning in tests.

Adding deprecation code for something in Drupal 8 for Drupal 10 should also be a rare occurrence too. Because it introduces more risk - we have to maintain the deprecation path in both Drupal 8 and 9.

Saying you're update to date with deprecations is not the same as declaring compatibility. If you want to trigger Drupal 9 deprecations then you need to declare that you are up-to-date with drupal:9.0.0 and then this deprecation will be triggered for you if you use the code that triggers it.

This is the case for JSON_API - it was compatible with Drupal 8.7.x it also triggered new deprecations. So in the future it would be able to add something somewhere to say I'm up to date with drupal:8.6.0 and then the 8.7.0 deprecations would ignored when testing. Once it has handled those deprecations it could raise this to drupal:8.7.0. At that point drupal 8.9.0 might be out and JSON API would be compatible but it is likely it will be triggering deprecations for 8.8.0, 8.9.0 and maybe 9.0.0 (if those come to pass).

As discussed with @alexpott in slack, what he defines as "compatibility" is the same as the minimally supported core version, which is typically defined as a version dependency on the system.module (since you can't depend on drupal aka core in info.yml at the moment):

From pathauto.info.yml:

- drupal:system (>=8.5)

That means pathauto is "allowed" to update all things that were deprecated in drupal:8.5.0 and earlier. And it can not do anything about things deprecated in 8.6 or any later version, including drupal 9. So we should not show those deprecation messages. It will still *run* in 8.6, 8.7 (hopefully..) but it will use deprecated code and will trigger hidden deprecation messages.

At some point, pathauto will update that information say I'm compatible with ">= 8.8 || >= 9.0". that means it will not work at all on 8.7 and anything longer. And we can show all D8 deprecation messages. But if 9.0 (more likely, 9.1) will add new deprecation messages, then pathauto will need to use the deprecated code as long as it supports Drupal 8. At some point it will then update that again, for example ">= 9.2", meaning it will no longer support Drupal 8 at all and can use new API's and non-deprecated things from 9.1 and 9.2.

Whether or not pathauto *works* on Drupal 9/10/8.8 doesn't need any deprecation-message at all, either the tests pass or not. So once Drupal 9 is actually available in some version and removed the deprecated code, I can just run the tests against that because our deprecation messages will not cover every single thing anyway (e.g. deprecated constants, which only static inspection tools like phpstorm/phpstan can point out automatically). And then as we continue to release minor version for D9, deprecation messages will become more interesting again.

Wit the exception of our technical dept of things that we are only now properly deprecating and removing our own usages (entity manager, entity_* functions, db_* functions, ..), "fixing" deprecation messages has a cost (beside having to do the work, obviously): you have to increase your required core version and if you forget to do that, you will get bug reports from angry users (happened to me quite often).

you have to increase your required core version and if you forget to do that, you will get bug reports from angry users (happened to me quite often).

This kind of thing is the reason that we should have min/max tests, and why we should have different test targets (release vs. issue, etc) #2951375: Allow for build targets

That way you would be equipped to have a special release test that verifies your declared minimum. The workaround is to have a patch to drupalci.yml that does all the things and then run it manually, but that's still too inconvenient.

Moving forward, having a consistent deprecation format is a good idea, and the ways offered here are reasonable. But if we have a coder rule, then it will fail many years'-worth of deprecations, and it would be a lot of effort to fix those. That effort would be better spent making sure each @deprecated has a @trigger_error().

If there were a way to only ever sniff changed files, then that would be reasonable. In fact, that's what the testbot does for patches, but it always sniffs the whole project if there isn't a patch. We could probably come up with a way to alter the phpcs testbot plugin so it behaves this way.

@Mile23, it's been the intent to add a coder rule for deprecation messages for years, and yes, we'll need to update existing code. But this is a good sprint task and many of them may be scriptable.

I think saying something is deprecated in Drupal 9 if we deprecated it in Drupal 8.8 is wrong. You should still kill your simpletests even if core isn't quite ready to kill it in a year. I don't see why our tooling couldn't filter the messages by the removal major version; that's the point of having a parseable format, after all. Humans will filter the message by major version, so tooling can too. It's no more difficult than parsing the version it was deprecated in -- it is, in fact, almost exactly the same.

If trigger_error() calls (or DeprecationError::generate()) will be parsed in the future, would it be possible to remove @deprecated tags in those cases where not the whole function/method is deprecated?
This is currently a bit annoying, when the IDE screams “This is deprecated!” but in fact only one parameter you’re not even using is.

Thanks everybody! This issue is in great shape for final discussion and is included in today's announcement: https://groups.drupal.org/node/534895

Depending on additional feedback from the community this will be evaluated again on or around March 20 for approval by the TWG, and if all goes well it will then be marked RTBC so it can be approved by a core maintainer.

From #38, #52 and #54 we appear to have broad agreement that the addition style of message should say "foo is deprecated" not the confusing "foo is supported ... and will be required in ..."

So should the issue summary proposal be updated to this? It still has the old style of message.

@jonathan1055 yes it does appear there is agreement on this, so the proposal can be updated to reflect it.

I have updated the issue summary based on the more flexible version proposed by alexpott in #54 which was

@trigger_error()
%thing% is deprecated in %in-version%. %extra-info%. See %cr-link%

There was some subsequent discussion about the need for parsing the version that the code will break in, but there was no clear outcome on that. If that is required then the proposal would be:

@trigger_error()
%thing% is deprecated in %in-version% and will break in %removal-version%. %extra-info%. See %cr-link%

For the docblock text I have changed it from removed before %before-version% to removed from %removal-version%. This has the benefits that (a) it tells developers that the code will break in that version (b) it is clearer than "before" because that could mean any time and any version after the in-version, which is confusing, (c) it is better grammar to remove something from a thing.

Rationalised the Notes into the Dedfinitions.

The period for final discussion has concluded. Since there have been no objections this issue can now be considered to be reviewed and accepted by the community. However since this is a change that affects Drupal core this needs to be signed off by a core committer.

I am moving this to the core queue so it can be approved by the core team. If no more changes are needed this can be tagged with "Core Committer Approved" and moved back to the coding standards queue.

After this is done we can update the respective pages to publish the changed deprecation format.

In the examples provided in IS, some @trigger_error() cases are ending with dot, some not. The template %thing% is deprecated in %in-version%. %extra-info%. See %cr-link% doesn't require a dot after %cr-link%. I think it should end with a dot because is not a docblock @see but a human-readable phrase.

It's consistent in fact, the one example that deviates from the template is the "Doing the wrong thing example" which shows how it should _not_ be done.

OK this already has plenty of input from multiple core committers, works for me too so tagging and moving back.

However claudiu.cristea's observation in #74 has not be answered or fixed. The @trigger_error() template and two of the examples do not end with a dot. The third example does end with a dot. Because we know the last part of the message is a url then we do not need a dot after it. I have ammeded the 3rd example in the IS to be consistent with the others.

Dot is valid char for url path, so better remove it or use space after url https://www.w3.org/Addressing/URL/url-spec.txt

PS raised this many times, very often text editor opens a page with dot at the end of url in browser
That will affect d.org when it will went to 8.x where node/123. leads to 404

Might want to check out how hooks are deprecated.

See: https://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Extension/M...

Thanks everyone for the interest and suggestions but please try to refrain from changing the proposed wording at this time. According to our procedure the text was finalized at the moment the deadline for final public discussion has expired and the ticket changed status to RTBC.

The standard has already gone through extensive discussion, and is formally approved by a core committer. If there are any important changes needed in the standard then this will need to go through the entire procedure again which will set us back by several weeks. The goal is to get this in by the time Drupalcon starts so this can be worked on at the sprints.

The standard is now considered final and will be included in its current form, except if a core committer steps in and removes the "Core Committer Approved" tag.

Thanks everyone for the interest and suggestions but please try to refrain from changing the proposed wording at this time.

Just to be clear, the proposed wording has not changed since 14 March (before RTBC). Comments #77 and #78 were just making the example match the standard format.

This can't be core-committer approved until the rule is done.

This is related: #2997100: Introduce a way to deprecate config schemas.

#2908391: Add a rule for expected format of @deprecated and @trigger_error() text is committed.

These issues are now open to fix core for the standards:
#3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard
#3048498: [≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard

The rule is implemented in Coder and has been released! We don't need to wait for core adoption to get the new standard published. @xjm, can we get the "Core committer approved" tag back?

I'll take care of updating the standard.

Having the text consistent is a good idea, and I raised the difference in #71. Finding a set of words which make sense regardless of whether the %thing% is a noun or a verb was the sticking point. Saying 'removed' does not work in all cases. So maybe "will not work in" or "will break in" is a general phrase that might be acceptable?

I'm happy to re-work the coder sniffs when there is agreement on the standard, that's not a problem :-)

OK so we seem to be heading back to requiring the %removal-version% in the @trigger_error standard text, not relying on it to be added in the %extra-info% section. It was there before, then we relaxed the wording, but I'm fine with adding it back in, and making the text copy/pasteable where a @deprecated tag and @trigger_error call are both used.

However, to allow for readable grammar when the %thing% being deprecated is an 'activity' not an 'object', such as calling a function without a parameter, we should let the @trigger_error text use "is not allowed" or "is disallowed" as an alternative to "will be removed from"

So the @deprecated standard stays exactly the same as we have now, i.e.

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

The @trigger_error standard would become:
%thing% is deprecated in %in-version% and will be (removed|disallowed) from %removal-version%. %extra-info%. See %cr-link%

This brings up one other quesion - when we say "will be removed" this is a future-looking statement which is only factually accurate fot a short finite period of time. As soon as the %removal-version% exists the statement is inaccurate. If we used the general "is removed from" then this reads OK before the time of removal, and will remain accurate from a certain point in time forward for ever.

So my suggestions for the @deprecated tag standard is:

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

and the @trigger_error standard would become:
%thing% is deprecated in %in-version% and is (removed|disallowed) from %removal-version%. %extra-info%. See %cr-link%

Yeah I have the same question as #93.

I think "is removed from" is fine.

We can not always say is removed from in the @trigger_error() text. This has been shown in #56. Whatever standard we come up with needs to cope with all the @trigger_error use cases which are not just straight up deprecation.

Thanks alexpott, it was your comments from way back that I was catering for.

So as I suggested, if we have is removed or is disallowed in the trigger_error standard, that would cope with all cases.

[edit: cross-posted before seeing comment #96 above]

It is highly likely that matching @trigger_error with @deprecated will prove very tricky as it is possible for a method to have multiple @trigger_error and 1 @deprecated - hopefully this is super rare but over time this is a very likely scenario.

@jonathan1055 I'm not a huge fan of the word disallowed. For instance a case like:

@trigger_error('Calling Foo::__construct() without the $entity_repository argument is deprecated in drupal:8.7.0. The $entity_repository argument will be required in drupal:9.0.0. See https://www.drupal.org/node/2549139', E_USER_DEPRECATED);

doesn't really work. I worry that we are being overly prescriptive for little to no gain. We should always value clearly written docs and instruction over a standard that makes it easier for computers to extract strings. The message for the above trigger error is clear and correct. The the argument becomes required in Drupal 9 - saying anything else means that the developer reading that has to learn new terminology.

@Gábor Hojtsy I think your comment in #99 points towards a resolution. I think what we need now a standards proposals that we can test against the requirements which seem to be something like...

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

If an @deprecated tag is present there must be a corresponding @trigger_error. The @trigger_error() has the format
%thing% is deprecated in %in-version% and will be removed from %removal-version%. %extra-info%. See %cr-link%

For @trigger_error()'s without a corresponding @deprecated tag the format is:
%thing% is deprecated in %in-version% and will be VERB ADVERB %removal-version%. %extra-info%. See %cr-link%
OR
%thing% is deprecated in %in-version%. %extra-info%. See %cr-link%

So the above satisfies the requirement of matching text for @deprecated and @trigger_error and can possibly be enforced in the the @deprecated PHPCS check (note I don't think we can check anything about the @deprecated tag in the @trigger_error rule). The one thing to be decided is what to do about the @trigger_error()'s without a corresponding @deprecated tag case - which is why I keep bringing it up. I'm not sure what is best because the situations where is can be use are manyfold - hence the loose %thing% is deprecated in %in-version%. %extra-info%. See %cr-link% suggestion originally arrived at.

Just found

   * @deprecated in Drupal 8.6.0 and will be changed to a protected method
   *   before Drupal 9.0.0. There will be no replacement for it because the
   *   default table mapping is now able to be initialized on its own.

in web/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php

Unfortunately the current standard makes doing that hard.

@alexpott Thanks for summarising in #100 where we seem to have got to. However, you said:

If an @deprecated tag is present there must be a corresponding @trigger_error.

I don't know if this is correct or not, but it appears to contradict @Gabor in #17 on #2908391-17: Add a rule for expected format of @deprecated and @trigger_error() text

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.

and in #19 also from @Gabor

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.

I'm not trying to say which is correct, just pointing out that I think I'll hold off doing any relationship checking until we get this discussed and agreed.

@jonathan1055 good point. Given the just-now discussions on #2847678-33: Deprecate WebTestBase and its required classes in favour of BrowserTestBase I think you and @Gábor Hojtsy are correct. So given #102 and #103 the standard could be...

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

For @trigger_error()'s the format is:
%thing% is deprecated in %in-version% and will be VERB ADVERB %removal-version%. %extra-info%. See %cr-link%
If there is a corresponding @deprecated tag the VERB ADVERB is expected to be removed from

So that leaves us with how to address #101. I guess one question is - Is #101 is a reasonable usage of an @deprecated tag? I can see both sides of that - on one hand the function is not deprecated - only it's scope is changing - on the other hand how to do this if you don't use @deprecated. Actually I think that helps answer the question. Since an @deprecated tag cannot differentiate how a method is called we should remove this and replace it with an @trigger_error that looks at the caller OR we create a new protected method now and do a proper deprecation.

I still think it might be worth being less prescriptive in the standard for @trigger_error and making it something like %thing% is deprecated in %in-version% SOME TEXT %removal-version%. %extra-info%. See %cr-link% because I think we're in danger of trying to over prescribe and will end up with a standard that forces bad choices on us. I also think %extra_info should be optional.

[Edit: to correctly attribute correctness and a word on optionality.]

I have updated the proposed standards in the issue summary to reflect the comments from #89 onwards.

Specifically where a trigger_error can be associated with a @deprecated doc tag this implies that there really is a "removal" and the more strict format is required, which will make it the same as the @deprecated text. Where no associated @deprecated tag is found, a more relaxed standard is used which allows free text to describe the nature of removal but still requires the removal version.

Secondly, as per the suggestion in #92 I have changed the future-looking 'will be removed from' to 'is removed from'. xjm and alexpott have both used this phrase in comments above.

Even if the wording is not yet finalised, it seems we have agreement on the need to have both strict and relaxed trigger_error versions depending on whether an associated @deprecated tag can be found. I have started work on this and will update #2908391: Add a rule for expected format of @deprecated and @trigger_error() text with a patch in due course, so that technical discussions can continue on that issue, so that this issue can remain focussed on agreeing the standards.

There is a patch for review and a pull request on
https://www.drupal.org/project/coder/issues/2908391#comment-13094468

@klausi has proposed the next steps forward for this, but it would be good to get final approval on the changed standard for trigger_error which aligns it with deprecated tag for the strict cases. What is the process for getting this approved? Can we just get confirmation from key people involved here such as @gabor, @alexpott, @xjm, @pfrenssen?

Thanks @catch, that's good. We also now have Core using the latest coder 8.3.4 so the modified sniffs can be used on
#3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard and
#3048498: [≈Nov. 11] Fix Drupal.Commenting.Deprecated coding standard

I have made the preliminary updates to the deprecation policy page https://www.drupal.org/core/deprecation so that the examples match the new agreed standard. It needs to be expanded to explain the strict vs relaxed trigger_error layout, which I can do but have not got round to that yet.

Added remaining tasks to the I.S.

Secondly, as per the suggestion in #92 I have changed the future-looking 'will be removed from' to 'is removed from'. xjm and alexpott have both used this phrase in comments above.

As a non-native speaker I found this to be puzzling, since "is removed from" tells me that the removal already occurred, but in reality we're referencing a future unreleased branch (which might not even be called the way we're documenting it, since it's not a given that the 9.x prefix will be used, but that's a different story). The "will be removed" version sounded more logical. Just my two cents.

Hi bojanz, thanks for your interest, and I will try to explain.

"is removed from" tells me that the removal already occurred, but in reality we're referencing a future unreleased branch

You are right that some of the deprecation texts refer to future events, but many of the existing ones refer to past events where the deprecation has already happened. Often the most critical time for developers, when we need to know about these deprecations, is when the deprecation has just occurred (because we did not see it in advance) and something needs to be fixed. At that point, the event is already in the past.

Secondly, if the code branch referenced in the message already exists then it is not a future event, that branch is useable and the function/feature has already been removed from it.

Thirdly, any future event will become a past event in due course, hence any specific deprecation will eventually become a past event.

So we took the decision to use a form of language that is neither forward-looking nor backward-looking but just states the situation from a neutral point.

Also not a native speaker, and the last thing I want to do is change the message again but I also don't really understand it, also not after that explanation :)

Arguably past/present/future is relative in this context. If you are reading this message then this means that *your* present is Drupal 8 and Drupal 9 is *your* future, whether or not that has technically already been released/done. So to me, that made more sense.

But again, I care 10x more about just getting our deprecations cleaned up and ready for D9 than grammar ;)

From #107 Issue tags: +Core Committer Approved

From #108

I have made the preliminary updates to the deprecation policy page https://www.drupal.org/core/deprecation [...] It needs to be expanded to explain the strict vs relaxed trigger_error layout ....

I have updated https://www.drupal.org/core/deprecation#how with the details taken from the top of this issue.

#3094454: Fix remaining @deprecated manually and enable the coding standard is RTBC, then we can work on #3122186: Fix @deprecated versions and enable Commenting.Deprecated.DeprecatedVersionFormat sniff

This future looking, present looking, past looking it is getting so confusing the more I think on this...

One suggestion:

"DEPRECATION MESSAGE WILL ALWAYS MAKE SENSE FOR OLDER VERSIONS"

The deprecation message are always going to be for the old versions only (we will never have a deprecation message for a yet to release version) where in the deprecated functionality will surely be removed from some future version, whether that future version is released or not... and that future version obviously is not released when deprecation was decided!
hence to me it always make sense to say "will be removed" instead of "is removed". Even if that future version releases, it still makes sense for that old version to say 'will be removed' from some future release/version...

for this deprecation message will never be there in the version where deprecation was to be removed.

From this standpoint of the older version where deprecation happened, it is always going to be a future thing for the deprecation to be removed, so I believe and profess that:

"WILL BE REMOVED" makes more sense than using "IS REMOVED"

#110 ++

This I believe certainly needs another perspective to look at

we may have something like

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

OR

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

OR

@deprecated in %deprecation-version% and will remove from %removal-version% onward. %extra-info%.
@see %cr-link%

sharing for thoughts.

I have updated the API documentation, @deprecated: Indicating deprecated functionality so it will now need a review.

The api documentation updates are now clear, I think this issue is done now?

@borisson_, thank you!

I have updated the IS to show that all but one of the remaining tasks is complete. As per practice, I am marking this fixed because there is only one task left. That issue needs to be rerolled for 10.0.x, if anyone is looking for something to do.

Thanks everyone!

@jonathan1055 let me know that I didn't update credit. I am assigning this to myself as a reminded to do that this week.

Finally got back to this. Adding credit. Let me know if it needs further changes.

Oops, too late. All that work lost. I'll get some help.

Reopening this issue per request from @quietone.

Adding credit per How is credit granted for Drupal core issues.

Came back and I see the credit is still not granted. :-(

Right, I am not able to add credit!

I am setting this is NW only sorting out how to add credit.

Assigning back to myself as a reminder to continue to work on getting the credit fixed on this issue.

Let's see if I can add credit now.

It seems to have worked this time. I followed the How is credit granted for Drupal core issues policy.

I am restoring the Fixed status and un-assigning.

For posterity, we temporarily granted @quietone "maintain issues" on the coding standards project since none of the maintainers were available ensure credits. (This seemed like a reasonable compromise given the role the release managers have in helping facilitate the discussion and implementing standards like these.) Belated thanks to everyone who helped make this a reality!

For posterity, we temporarily granted @quietone "maintain issues" on the coding standards project ...

@quietone is doing a lot of very good work overseeing and managing coding standards both here and on slack meetings, so I'd like to recommend that they be given the "maintain issues" permission permanently.