Problem/Motivation
When adding a new deprecation, it is tempting to add the message to \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() instead of resolving all of the deprecations in the initial patch.
However, this is not the intention of that list, and will only expand technical debt.
Proposed resolution
Adjust the docblock to indicate why the list exists and that new things shouldn't be added.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 3002935_6.patch | 941 bytes | mile23 |
Comments
Comment #2
tim.plunkettComment #3
alexpottTempting to get meta and mark the method @deprecated :)
Thanks for opening this issue. +1 to documenting our expectations.
Comment #4
berdirAlso a bit related #3002148: Support deprecation testing for multiple branches on contributed modules on Drupal.org (Point 1 would be a version of that by simply not adding a @trigger_error(), I'd rather have the @trigger_error() + the exclusion than that, but agree that it's best to not have deprecated calls at all).
Two questions/cases to consider:
* #2910353: Prevent saving config entities when configuration overrides are applied, as you can see, that causes hundreds of test fails due to the deprecation message, a lot of them are one-off cases in tests that load a config entity and save it again. How are we going to handle that? Converting all in a single patch does not seem feasible. Only add the @trigger_error() everything in core has been converted? So (at least) 3 issues: 1. add the new stuff and example conversions. 2. convert everything (maybe multiple issues). 3. add the @trigger_error(). Downside of that is that its easy to ignore/forget about those soft-deprecations.
* Previously deprecated stuff like entity.manager. I'm working on getting rid of old usages a bit, e.g. #2624770: Use more specific entity.manager services in core.services.yml is now RTBC, but there's a ton of work left and then eventually #2886622: Deprecate all EntityManager methods with E_USER_DEPRECATED would want to add @trigger_error()'s to all entity manager methods. (we can't deprecate the service/class completely as we automatically inject it into entity handlers for example). Do we need to convert every single usage in core before we're allowed to add the @trigger_errors()? Maybe existing deprecations should be excluded from this rule? At least that would give us a single place that lists what's left to do.
Comment #5
tim.plunkettFinal call is up to the framework/release managers.
But I would rather break this rule in extreme cases, instead of *not adding @trigger_error*
Comment #6
mile23Adding a message to the ignored deprecation list is the same as not adding @trigger_error(). For instance, patches in review process that call the old methods will still pass and get committed, adding technical debt.
So upholding the principle of not adding errors has the same result as breaking that principle, in the short term. In longer terms, the principle wins because we reduce technical debt with less work by failing newer patches.
A deprecation process that doesn't understand its scope probably isn't a good move... If you're failing a thousand tests then you should have a plan. You can make a meta that addresses how to make the change in workable and reviewable chunks, with the last step being 'Add @trigger_error().' In that case, adding the deprecation in one step, working on usages in another, and then adding the error is pretty reasonable, as long as there's a plan to do all that.
So as someone who's making his way down the list in #2959269: [meta] Core should not trigger deprecated code except in tests and during updates and sweeping up after folks, I have to say... Please don't add to it, or if you do then have a plan in place with existing issues and @todos in code, with inline comments in getSkippedDeprecations() explaining why. You should have those things anyway, so just add an 'Add @trigger_error()' issue to cap it off instead.
Comment #7
mile23Comment #8
tim.plunkettStill not 100% sure on when we'll actually break this rule or not, but the documentation of our intentions is good.
Comment #9
alexpottCommitted and pushed 12d8100f57 to 8.7.x and b2afca3467 to 8.6.x. Thanks!
Backported to 8.6.x as it is a docs change.