Rework deprecation messages to stop mentioning the minor branch

Created on 8 May 2023, over 1 year ago

Problem/Motivation

When we deprecate things, the deprecation message includes the deprecated in version and the removed in version. However many other projects only put the removed in version.

There are a couple of disadvantages to the current system:

1. When we start a new minor branch, active patches become outdated, since they need to update the deprecation to the new version.

2. There is also ambiguity - i.e. during alpha, if an issue gets committed, it might not need to be updated, but then after beta rc it probably will. But in some cases we backport deprecations to betas, rcs and even patch releases if the deprecation is low disruption and there's an important bugfix. This has on occasion taken multiple commits to resolved.

Steps to reproduce

Proposed resolution

Before:

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

After:

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

Or..

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

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

🌱 Plan
Status

Active

Version

10.1

Component
Base 

Last updated about 5 hours ago

Created by

🇬🇧United Kingdom catch

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @catch
  • 🇬🇧United Kingdom catch
  • 🇬🇧United Kingdom catch
  • 🇳🇱Netherlands spokje

    I like this, especially for reason #1.

    Would like to see the 2nd option implemented, so @deprecated in %deprecation-major-version% and is removed from %removal-version%. %extra-info%. @see %cr-link%.

    This because of deprecations we make but can't always completely remove before the next major (looking at you Call to deprecated constant REQUEST_TIME..).
    Implementing option 2 would give us some more history for basically no(?) extra cost.

  • 🇬🇧United Kingdom catch

    fwiw I also personally prefer the second option. There would occasionally be deprecations that miss a major, but patches have to be updated once every two years anyway.

  • 🇳🇿New Zealand quietone

    Would there be a corresponding change to the @trigger_error*() message format?

  • 🇬🇧United Kingdom catch

    @quietone yes we'd need to update both the phpdoc and the trigger_error(), although if we go with option 2 it would only be the removed in version changing from 10.2.0 to 10.x

  • 🇬🇧United Kingdom catch
  • 🇧🇪Belgium borisson_ Mechelen, 🇧🇪

    I agree, the second option here seems like a good way to resolve the problems with unneeded rerolls.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    +1 for the second option

  • 🇺🇸United States dww

    +1 for this. Bumping to at least NR. It's only a plan, so the only thing to review is the summary. Seems like universal support for dropping the minor branch version. Should this really be RTBC? What remains to decide this?

    Thanks!
    -Derek

  • 🇳🇿New Zealand quietone

    In #7 @catch says that the string would change from 10.2.0 to 10.x. So, the existing string

    drupal_common_theme() is deprecated in drupal:11.1.0 and is removed from drupal:12.0.0.

    would become

    drupal_common_theme() is deprecated in drupal:11.x and is removed from drupal:12.0.0..

    That is confusing with 11.x being the name of the main development branch. That could be avoided by waiting until Drupal 12 is released, or we remove the suffixes so it would be drupal_common_theme() is deprecated in drupal:11 and is removed from drupal:12.0.0..

  • 🇬🇧United Kingdom catch
    drupal_common_theme() is deprecated in drupal:11 and is removed from drupal:12.0.0
    

    Yes this is better I think.

  • 🇳🇿New Zealand quietone

    Thanks.

    Testing with that string does result in a phpcs warning

     1815 | WARNING | The deprecation-version 'drupal:11' does not match the lower-case machine-name standard: drupal:n.n.n or project:n.x-n.n or project:n.x-n.n-label[n] or project:n.n.n or
    
  • 🇬🇧United Kingdom catch

    Yes we'll need to change the coder rule. As far as I know we don't actually have a coding standard for this that I could find. I think the rule is based off the bc policy documentation here: https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... so for me this issue should be enough to change the policy (which we can then reflect in the docs), and then an issue against coder to relax the rule to allow it.

  • 🇳🇱Netherlands bbrala Netherlands

    Hmmz, althoough i understand the need to do this, i want to note that finding the minor it deprecated in will be a little harden. Although the CR will still mention it, when making rector rules to fix deprecations it means always checking the CR. Sometimes to support the new and old way of a deprecation there is no easy way around that and you need to up the version to the version where it was deprecated.

    This is not really a big issue, but a reality. This would also mean that when looking at deprecation messages you only get the major which might be an issue when digging into why a deprecation is triggering. Expecially in contrib. Steps to find out would then be;

    find the line in core, git blame, find issue, (and maybe find CR) to see what minor broke this.

    It's a trade off, but i understand what this would bring since it will lower the amount of adjustments and rework of deprecation messages.

  • 🇺🇸United States dww

    We never remove API changes in a minor or patch release, right? Should it just say:

    Deprecated in drupal:11 and removed in drupal:12

    ?

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    I'm actually also in favor of #19. We may deprecate things in any minor, but will only ever remove them in a major. If we're going to drop the minor version for when we deprecated something it makes no sense to me that we'd still specify the full version string for the next major. We'd never remove a deprecation in 12.3.1 for instance.

  • 🇬🇧United Kingdom catch

    Yes that makes sense to me too, we'd never even say 12.1

    Even if we made a mistake and left some code in 12.0 that should be removed, and removed it in 12.1 because it was blocking something else, even then we wouldn't go back to 11.x and update the message to say 12.1 because it was always supposed to be removed in 12.0.

    And if we loosen the requirement rather than preventing people from adding minor version information, it wouldn't prevent some kind of edge case anyway (like maybe deprecating and removing something in a minor for security hardening?).

  • 🇫🇷France andypost

    With PHP 8.4 and Add symfony/polyfill-php84 and make use of new array functions Needs work there's new [#Deprecated] attribute with since argument, so would be great to discus it in Adopt #[Deprecated] attribute Active

    #[Attribute(Attribute::TARGET_METHOD|Attribute::TARGET_FUNCTION|Attribute::TARGET_CLASS_CONSTANT)]
    final class Deprecated
    {
        public readonly ?string $message;
     
        public readonly ?string $since;
     
        public function __construct(?string $message = null, ?string $since = null) { /* … */ }
    }
    
  • 🇳🇿New Zealand quietone

    Updated IS.
    Would we then be allowing the following styles?

    @deprecated in %deprecation-version% and is removed from %removal-version%. %extra-info%.
    @see %cr-link%
    @deprecated in %deprecation-version% and is removed from %removal-major-version%. %extra-info%.
    @see %cr-link%
    @deprecated in %deprecation-major-version% and is removed from %removal-major-version%. %extra-info%.
    @see %cr-link%
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Re #23 exactly, but the emphasis should be on the last one. We can keep old messages as is without tripping up the code sniffers, but for the sake of maintainability should switch to the new format going forward.

    If we ever run into a case where we feel like it's crucial to mention the minor and/or patch version for the deprecation, we can. But I can't think of any scenario where that would be crucial information. The CR we have to link to usually contains that extra piece of information anyway.

    So I'd suggest we go with #23.3 and open a follow-up for D12 where we only allow that format from that point on.

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • 🇨🇭Switzerland berdir Switzerland

    I'm not sure about this.

    I get the benefit to core merge requests but to me as a contrib maintainer, the minor version it is deprecated in is important. To clarify that, it's not the deprecation that's important, it's when it is safe to use the replacement.

    Sometimes we deprecate stuff in favor of something that's already available. For example the entity query access stuff existed since 8.0 so people sometimes got confused and thought using that would require the minor version it was deprecated in, but it didn't.

    But in most cases, deprecation and introduction of the replacement API is the same minor version. That minor version is an important factor on what minor version a certain contrib module requires, whether or not it's worth using the DeprecationHelper or custom code for something similar. Or just postpone it, for example 📌 Replace usages of deprecated RendererInterface::renderPlain() Active , which again also depends on when it will be removed.

  • 🇺🇸United States dww

    As usual, I believe @berdir speaks the truth. However, this need:

    To clarify that, it's not the deprecation that's important, it's when it is safe to use the replacement

    is not addressed by putting the exact version the deprecation was added in the messages. You need to read the CR and understand the change to know when the replacement was available, as you explain. But yes, in most cases, it is the same version. So yeah, it's hard to know what's best.

    I personally tend to maintain my projects to be as compatible with legacy versions as possible. Sometimes it's impractical and we need a clean break, but often I've found it's barely any technical debt to be able to keep working with very old versions of core. I read the CR, figure out when the replacement was added, add some custom code to call the right thing via `version_compare()` and move on.

    I'd be happy either way. I see the benefit of knowing the specific version the deprecation was introduced. I'd be glad to not have to push new commits to open MRs all the time to keep them valid.

    Either way, it generally seems pointless to say "is removed in 12.0.0" per #19. So if we keep minor version in there, I'd vote for the middle of the options in #23:

    @deprecated in drupal:11.1.0 and is removed from drupal:12. %extra-info%.
    @see %cr-link%
    
  • 🇮🇹Italy mondrake 🇮🇹

    My 2 cents:

    - minor is important for knowing exactly since when new code should be used, for the reasons explained ^^. Patch is not relevant here.
    - major is enough for removal

    so I'd suggest

    @deprecated in drupal:11.1 and is removed from drupal:12. %extra-info%.
    @see %cr-link%
    
Production build 0.71.5 2024