Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The new deprecation policy adds an expectation that @deprecation
's will also trigger an error. Core should comply.
Proposed resolution
Add trigger_errors for all deprecations and ensure that they are tested.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff.txt | 1.27 KB | Mile23 |
#23 | 2856744_23.patch | 10.37 KB | Mile23 |
#21 | interdiff.txt | 3.55 KB | Mile23 |
#21 | 2856744_21_deprecation_test-do-not-test.patch | 3.54 KB | Mile23 |
#21 | 2856744_21.patch | 9.66 KB | Mile23 |
Comments
Comment #2
xjmThis is postponed on #2856742: [meta] Adopt trigger_error() for deprecation messages where it is missing and possibly #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation.
Comment #3
xjmComment #4
xjmComment #6
larowlanDoes this need to remain postponed on #2856742: [meta] Adopt trigger_error() for deprecation messages where it is missing ?
I understand this one is about updating existing deprecations, but is there any reason we can't be fixing those that don't have trigger_error at the same time?
Comment #9
BerdirOne year later without comment... I don't think so :)
Not sure what the testing part is about, but I think the idea is that we test that they are *not* triggered, so the test support we have is sufficient.
Comment #10
alexpott@Berdir the testing part is that we ideally should have at least one legacy test to prove the code still works and is not broken by later changes - until we move to Drupal 9 and can finally get rid of it.
Comment #11
Mile23Here are two patches:
One is a test you should run manually and which shouldn't be part of core. It loads every non-test PHP file, checks if it's a class with @deprecated annotations, requires the file, and then verifies that it triggered an E_USER_DEPRECATED error. It requires symfony/finder. This is updated from #3007329-7: [META] Drupal 8 core must not use any deprecated code, must be enforced by automated testing which I should have posted over here.
The other patch adds all the @trigger_error() calls that the test says we should add.
Let's see what technical debt is revealed.
Comment #13
Mile23There will be approximately eleventy-billion fails on that last patch.
That's because EntityHandlerBase hasn't been replaced in core: #2471663: Undeprecate EntityHandlerBase
This patch undoes the @trigger_error() for EntityHandlerBase.
Don't commit FindDeprecatedTest. :-)
Comment #15
Mile23So, uh... golly. The messenger service is a very weird deprecation. There should be a followup where someone either explains why
\Drupal::messenger()
saysnew LegacyMessenger()
or changes core.services.yml to useLegacyMessenger
.Anyway, we skip
LegacyMessenger
because of the complexity of the deprecation.Comment #16
Mile23Comment #18
Mile23Added child issues: #3009848: Fully deprecate WorkflowDeleteAccessCheck #3009854: Fix "The "serializer.normalizer.file_entity.hal" normalizer service is deprecated: it is obsolete, it only remains available for backwards compatibility." deprecation error
Added a few skips to the test, documenting why.
Comment #20
Mile23Does TestBase actually need its deprecated TestServiceProvider? Let's find out.
Comment #21
Mile23OK.
This patch splits out the test and stores it here for posterity, and also gives us a patch that we can use.
The caveats here include:
This test only finds classes with @deprecated but no @trigger_error().
It excludes searches for plugins, because plugins are supposed to say @trigger_error() in their constructor. That will require a different test.
It doesn't find anything else that could be further deprecated. So that leaves all the other stuff from https://www.drupal.org/core/deprecation
Therefore this patch is incremental.
Comment #22
alexpottThis needs an
as KernelTestServiceProvider
- we've had problems before with specific PHP versions and namespace clashes.Comment #23
Mile23Using 'as'.
Comment #24
BerdirDoes this actually make sense?
Isn't the new one only for phpunit-based tests?
TestBase and its children will at some point get the @deprecated message themself and will be removed completely?
Comment #25
Mile23It makes sense because the goal is to add @trigger_error() everywhere it belongs:
Comment #26
BerdirOk, I see, I was confused because you're doing a "use as DifferentClassName" and KernelTestBase doesn't do that and still references TestServiceProvider. Kinda weird that this setter is in TestBase but only KernelTestBase uses it?
Are you sure the "use as" is really necessary as apparently KernelTestBase, which is in the same namespace, doesn't need it?
Comment #27
BerdirAlso, this currently "only" adds @trigger_error() to completely deprecated classes, but it says "to code". Which I guess is going to be a lot of places, so maybe we need this to be a META and your patch would be a "to deprecated classes" issue?
Comment #28
Mile23See #22.
And yah, this should be a meta with different scopes below it.
Comment #29
BerdirMaking this a META then.
More related issues:
#3019834: Add @trigger_error() to deprecated url/link EntityInterface methods
and as a special case of that: #2402533: Provide File::createFileUrl() as a replacement for the deprecated File:url() implementation
Comment #30
Mile23Added child: #3020849: Add @trigger_error() to @deprecated classes in a semi-automated way
Comment #31
Gábor HojtsyHm, how is this different from #2856742: [meta] Adopt trigger_error() for deprecation messages where it is missing ? Should we close that one in favor of this?
Comment #32
Gábor HojtsyRe-parenting.
Comment #34
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented#2908391: Add a rule for expected format of @deprecated and @trigger_error() text has been committed, so if you are using the latest coder dev release it will check against the standards as agreed on #3024461: Adopt consistent deprecation format for core and contrib deprecation messages
Comment #39
quietone CreditAttribution: quietone as a volunteer commentedThanks to everyone who worked on this.
This is for deprecations that were removed in Drupal 9.0.0. 'Tis now outdated. Therefore, closing as outdated.
Thanks!