Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#2858482: Simplify REST routing: disallow requesting POST/PATCH in any format, make consistent added lots of skipped deprecations to DeprecationListenerTrait::getSkippedDeprecations()
.
The missing API to deal with this more elegantly (to allow expected deprecations that aren't hardcoded in docblocks: ExpectDeprecationTrait::expectDeprecation()
) was added in #2935755: Add a trait to allow dynamic setting of expected deprecations.
Proposed resolution
Remove #2858482's additions to skipped deprecations, instead use #2935755's new API.
Remaining tasks
Patch.- Get green.
- Review + RTBC.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#29 | interdiff-29.txt | 1.04 KB | amateescu |
#29 | 2936704-29.patch | 33.22 KB | amateescu |
#26 | 2936704-26.patch | 33.64 KB | Wim Leers |
#26 | interdiff-20-26.txt | 8.04 KB | Wim Leers |
#20 | interdiff-20.txt | 674 bytes | amateescu |
Comments
Comment #2
Wim LeersThis is what we want to remove (this will fail, because I'm not using the new API yet).
Comment #3
Wim LeersThis is what the fix should look like, by using the new API.
Comment #4
Wim LeersUnfortunately (but understandably and reasonably!) we also need to add
@group legacy
… everywhere.Comment #5
Wim LeersWhen I run this locally, I get the following error when using the trait that #2935755: Add a trait to allow dynamic setting of expected deprecations introduced:
Comment #9
alexpott@Wim Leers make sure your core/phpunit.xml looks something like that ^^
Comment #10
Wim Leers#9: it does look like that.
You can see in the failures above that my local environment results in the same error as on DrupalCI.
(I did forgot to add a few
@group legacy
lines, fixing that here.)Comment #12
Wim LeersWe (@alexpott, @Berdir and I) discussed this on IRC:
Comment #13
Wim LeersSo, in this patch:
EntityResourceTestBase::testGet()
EntityTestResourceTestBase::testFormatSpecificGetBcRoute()
for testing the deprecationEntityTestResourceTestBase::testNoFormatSpecificGetBcRouteForOtherFormats()
for testing that only the appropriate BC routes are createdentity_test
entity type, running it for all entity types is overkill. They do run for all formats though!@group legacy
Comment #15
alexpottI'm not sure this is correct. The new route seems to have GET on the end...
rest.entity.%s.GET
Also #2936802: expectDeprecation() doesn't work in isolated tests should allow this to work.
Comment #16
Wim LeersObviously you're right — I got that wrong in my copy paste haste. Didn't help that I can't run the test, of course!
Thanks for #2936802: expectDeprecation() doesn't work in isolated tests!
Comment #17
Wim LeersThis is now blocked on #2936802: expectDeprecation() doesn't work in isolated tests.
Comment #19
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#2936802: expectDeprecation() doesn't work in isolated tests got in last week so this is no longer postponed.
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#16 needed a reroll and a small CS fix, seems to be working fine locally.
Comment #21
Wim Leers❤️ Thanks @amateescu!
Comment #22
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNow.. looking at the patch a bit:
Is there any reason to have these as separate test methods? In practice, this means we add (2 * number of rest entity types) browsers tests, which are quite slow.
Comment #23
Berdir@amateescu: This tricked my as well, but note that they are actually on Entity*Test*ResoureceTestBase, which means we "only" run those for the entity_test entity type tests.
That said, that's still about 14 different tests classes for different formats and special cases like date related tests. So should we maybe have them on just one test or 2-3 for different formats or so?
Comment #24
Wim LeersBecause Alex Pott requested that. Quoting relevant bits from chat log in #12:
Comment #25
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Wim Leers, ok, how about implementing @Berdir's suggestion and provide these methods only in 3 classes, for example
EntityTestHalJsonAnonTest
,EntityTestXmlAnonTest
andEntityTestJsonAnonTest
?Or maybe a new subclass of
EntityTestResourceTestBase
and change the test methods to not look atstatic::$format
but loop through all 3 formats in one place?Comment #26
Wim LeersSounds good! To not have 3 identical implementations of both test methods in those 3 classes, I moved both test methods into a trait that all 3 classes use.
Comment #27
Wim LeersI see I didn't respond to the second option suggested in #25:
That would result in non-isolated tests and therefore potential side effects interfering, because we're testing something that depends on configuration.
That's why I implemented the first suggestion of #25.
Comment #29
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#26 looks great to me. This should fix the fails.
Comment #30
Wim Leers😱😅
STUPID STUPID STUPID Wim 😆
Thanks, Andrei!
Comment #31
alexpottAdding credit.
Comment #32
alexpottCommitted and pushed d4e8416189 to 8.6.x and 0934fdc54a to 8.5.x. Thanks!