[policy, no patch] Clarify deprecation policy 3.2

For me there are three reasons not to have tests that only test deprecation messages.

1. We increasingly require deprecations (instead of direct changes/removal) for code that is technically @internal - 'best effort' deprecations for constructor arguments, protected methods on a controller etc.

It is quite low-effort to add @trigger_error() and @deprecated for those cases, adding a test too would make it more onerous when the deprecation itself isn't strictly required.

If we required deprecations for non-internal code but not for internal code, then it adds a lot of overhead figuring out whether a specific method counts as internal or not - especially since we rely on the bc policy docs more than @internal/@api.

The best-effort deprecations have been shown to reduce pain for contrib because we know people often rely on @internal code even when we advise against it. Testing of deprecation methods has not yet been missed. I would not feel as comfortable asking for 'best effort' deprecations if that also meant asking for test coverage.

2.When we used to add deprecation testing, especially around 9.4/9.5/10.0, I think several times I was committing a patches to Drupal 10 adding a 9.x deprecation + test so it could be backport to 9.x as-is, then later, sometimes the same day/week, committing a patch to Drupal 10 to remove the deprecation and test coverage again. This always felt like busy-work and not actually helping anyone. Even over a longer time-span it's still adding dozens/hundreds of tests to then delete them again.

3. Sometimes we update deprecation messages - versions, maybe changing change record links to point to a different CR. If that message is asserted in test coverage then everything has to be done in two places.

I agree that we should not have those tests, I think they make more work than their value.

As a counterpoint, I think there are two things they could provide:
1. Ensure the deprecation was done correctly. I think this is taken care of by the code sniffing on the deprecations. the formatting has to be so specific I think you have to get it right.
2. It can make it easier to spot if someone removes a deprecation because they have to remove the test too. I almost didn't even put it here because if the reviewer and the committer missed that deprecation was removed they would probably miss a test being removed too so this is super edge case I think.

I still land on the side that the policy that they do not exist is more beneficial, but wanted to mention the counterpoints I think I've seen.

@nicxvan, Thanks for adding suggested text, that makes it much easier to evaluate.

I think that the intent of the current text and the suggested text are the same, so why change it? Also, the suggested text uses 'must' which is not common in our policies, so I would need more time to consider it. And without the 'must' there is room for interpretation and to request a test in some, as yet unknown, situation. That is something I value in this situation. So, I don't yet see the need for a change.

And with my educator hat on, I think there are better solutions. Such as, to have the discussion on the issue, or in Slack. That can be valuable learning for the contributors commenting and for those reading. And, there is always the option to defer the question to a core committer.

I agree that the intent is the same, but the current wording can be interpreted as it is not needed, but sometimes it can be added.

As in the issue summary I've been seeing some more issues with deprecations get a request for that test explicitly and seen it make more work or delay cause they then need to be removed before committing.

I did discuss this in slack and the line was interpreted very differently by someone else so I opened this to get clarity.

My main goal is to avoid issues waiting in needs review for a test being requested that isn't needed. I can always add it, but then it makes more work to remove it if the committers request it removed.

If the intention is to keep the current meaning as I interpreted then disambiguation would be helpful.

If it's intentional that it can sometimes be required then I can add the requested tests.

I have reviewed this again and I still conclude that no change is needed. Point 1 of that section clearly states the only situation where a test is needed.

However, others disagree. So, since #2 can be considered an example, how about doing the following?

• Tests.
  1. A test is only required when there is backwards compatibility (BC) logic that needs to be tested.
     Examples:
     1. Add a test when
        1. There are multiple code paths and the code that is not considered '@internal' according to the BC policies.
     2. Do not test;
        1. A constructor BC.
        2. That the deprecation error is triggered.
  2. When adding a test, use a @group legacy annotation in conjunction with calls to $this->expectDeprecation().

Seems straight forward enough.

#6 isn't in the issue summary so it's not clear to me what's being RTBCed here.

Ah, my mistake. I had intended to ping @nicxvan and forgot.

I have moved the changes to the issue summary. Since there are no changes to the intent I am restoring RTBC.

I think we should be a little bit future proof here - @group legacy annotation will have to be dropped soon. So I would suggest to mention using the #[IgnoreDeprecations] attribute as main direction, and leave the use of the annotation in brackets with info that it will have to be dropped before we hit PHPUnit 12.

@mondrake, There is no need to future proof the document. I think adding extra information can lead to confusion for developers and reviewers. As a 'how to' it should explain what to do now, not the future. Instead , the issue that is dropping the usage should be tagged for documentation updates with a task of updating the 'how to deprecate' page. And when that issue is committed the doc can be changed to reflect the new 'now'.

... you can (and in fact should) use #[IgnoreDeprecations] right now. There's plenty of examples in HEAD already.

Looks good to me thanks @quietone!

@mondrake, thanks for pointing that out. I must have made a typo when searching core. Anyway, adding that makes sense. I'd like to keep this issue in scope about 'when to add tests' and get this one done. And, since there are already several issues about the use of @group I'd like to take a few days and review those to determine how to proceed. Since several of those are in Coding Standards and I've put them on the agenda for the next meeting so I am not making a followup just yet for this.

@quietone, fwiw the issue that would unblock broader usage of attributes vs annotations in core is 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active . That would allow using #[Group('bar')] attributes in place of @group bar annotations in particular, which is causing fatal errors right now.

@mondrake, thanks for the info!

After a nights rest I looked into resolving the use of '@group legacy'. To address this I tracked down the issue where the ability to use the attribute was added. I think that documentation updates should have happened as part of that issue, but I missed that one at the time. I then updated the doc → and added a comment 📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Fixed on that issue.

Hopefully, that covers the point about using the attribute.

Thank you @quietone

@mondrake, your welcome.

I have updated the current and proposed text with the recent changes. @nicxvan has already agreed to the restructuring changes, which does not effect the change made for @group legacy. Therefore, I have updated the doc and closing this as fixed.

If I missed something, let me know.

Automatically closed - issue fixed for 2 weeks with no activity.