Closed (fixed)
Project:
Drupal core
Version:
9.0.x-dev
Component:
rest.module
Priority:
Normal
Category:
Plan
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
11 Jul 2017 at 11:18 UTC
Updated:
10 Feb 2020 at 17:59 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
wim leersThis removes all BC layers from
core/modules/restas of July 11, 2017, commite1e216138f5311a5dd6b96b5789ab2db9ac5fd5f.(Note that this must be applied on top of #2893795-2: Remove serialization.module BC layers.)
Comment #3
wim leers(This module likely has the most BC layers and the most BC layer test coverage of all of core I think, considering how much we can remove…)
Comment #4
wim leersRebased for 8.5.x, commit
0fa72bdeea3d72e874b25dc72d2185645b57d6c5.Comment #5
wim leersUpdating for BC layers added to #2720233: Use arguments resolver in RequestHandler to have consistent parameter ordering.
Slightly bigger diffstat now:
Comment #6
wim leersUpdating for BC layers added in #2825204: REST views: authentication is broken. Which added a whole bunch of BC layer/update path code that can be removed. ~180 additional line deletions:
Comment #7
wim leersThere have been quite a few REST changes lately. Patch didn't apply anymore.
Rebased on top of commit
91e668f5758159b27bea9e4bc3247969fa91232a, solved all conflicts. Because of removed whitespace, there are now fewer deletions:Also: I should've been queueing tests for this, to prove that it works. Did that this time.
Comment #8
wim leersMore REST tests have been added, I haven't updated them all perfectly, hence tests not being able to run.
5 more files changed:
Comment #9
wim leersThis should fix some of the failures.
Comment #10
wim leersPatch no longer applied, due to #2800873: Add XML GET REST test coverage, work around XML encoder quirks, #2905686: Expand EntityResourceTestBase to test PATCHing multi-value fields: both adding and removing items and #2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage landing since #9 was posted. Had to solve some conflicts.
Comment #11
wim leers#2765959: Make 4xx REST responses cacheable by (Dynamic) Page Cache + comprehensive cacheability test coverage added some extra code to ensure BC layers are respected by the support for caching responses:
(and more)
That can now all be removed too.
Diffstat keeps getting more appealing:
Comment #12
wim leersI hate
git diff --binarywith a passion. A) git is stupid for thinking it's binary, B) why can't I configure git to always create binary patches!?This patch is identical to 11, but with
--binary.Comment #13
wim leersDrupal\dblog\Tests\Rest\DbLogResourceTestfails with a fatal error, because it uses a deprecated base class that this patch is removing. Created #2927758: Update DbLogResourceTest to use the ResourceTestBase base class instead of the deprecated RESTTestBase to update that test to no longer use the deprecated base test class. That now blocks this patch from becoming green.Comment #14
wim leersSame for
ResponseGeneratorTest. Created #2927766: Update ResponseGeneratorTest to use the BrowserTestBase base class instead of the deprecated RESTTestBase for that.Comment #15
wim leersSame for
RestRegisterUserTest. Created #2927768: Update RestRegisterUserTest to use the ResourceTestBase base class instead of the deprecated RESTTestBase for that.Comment #16
wim leersThis is blocked on #13 + #14 + #15.
Comment #17
wim leersAll remaining test failures except for the 3 listed in #13+#14+#15 are like this:
Interestingly, this only happens for update path tests. My guess: because the default installation of D8 that update path tests run against does contain
rest.settingsconfiguration.If I restore just
then the error changes to:
Which points to
rest_update_8201()+rest_post_update_create_rest_resource_config_entities(). Sure enough, when I addrest_update_8201(), tests pass again. (Or at lestAggregatorUpdateTestdoes.) I searched and found the minimal work-around necessary for now.Comment #18
wim leersLooks like I literally have to restore the entire config schema.
Comment #19
wim leersThis fixes the failures in
EntitySerializationTest,Drupal\Tests\rest\Kernel\RequestHandlerTestandDrupal\Tests\rest\Functional\ResourceTest.For the 3 remaining failures, see #13+#14+#15.
Current tally:
Comment #20
wim leers#2927766: Update ResponseGeneratorTest to use the BrowserTestBase base class instead of the deprecated RESTTestBase landed. That still leaves #2927758: Update DbLogResourceTest to use the ResourceTestBase base class instead of the deprecated RESTTestBase + #2927768: Update RestRegisterUserTest to use the ResourceTestBase base class instead of the deprecated RESTTestBase.
Comment #21
wim leers#2927758: Update DbLogResourceTest to use the ResourceTestBase base class instead of the deprecated RESTTestBase is also in.
Comment #22
wim leersThe failing
RestExportAuthTestwas effectively an update path test, without it being named that way. Deleted it, and its test view.Comment #23
wim leersThis no longer applied. Rebased. Solved dozens of conflicts.
Diffstat is still similar:
Comment #24
wim leersPer #2940417: Remove always_populate_raw_post_data requirements check, which might be incorrect,
rest_requirements()can be removed as soon as Drupal doesn't support PHP 5.6 anymore. Drupal 9 won't support it. So, removing it.Comment #25
wim leers(The lower number of lines removed is mostly due to
RequestHandlerandEntityResourceTestBasehaving been improved in HEAD!)Comment #26
wim leersIt's been ~5 months since I rerolled this patch, so I forgot how stupid git is again:
Rerolled #24 with
--binary.Comment #27
wim leersFixed
New diffstat:
Comment #28
wim leersYay, only 30 failures, not bad! Fixing some of the trivial failures.
Comment #29
wim leersNo longer applies because #2910883: Move all entity type REST tests to the providing modules landed. Very painful rebase.
Comment #30
wim leersRebase completed. Hopefully I didn't make too many mistakes.
Comment #31
wim leersAnd while doing the rebase, I spotted a few things I missed in the REST tests that were added since #21/December last year.
More importantly, I again forgot to do #30 with
--binary🙃Comment #32
wim leersWith #2910883: Move all entity type REST tests to the providing modules committed, there now is again a whole lot of additional BC layer that needs to be removed! This interdiff does that.
Comment #33
wim leers#2927768: Update RestRegisterUserTest to use the ResourceTestBase base class instead of the deprecated RESTTestBase landed!
Comment #34
wim leers#2927768: Update RestRegisterUserTest to use the ResourceTestBase base class instead of the deprecated RESTTestBase landed, so this is no longer postponed on anything except for the D9 branch being opened 🥳
Nearly a year later, time for an update. Commit
c8dc57eedea3fd34de067098ca2ee36e6c459245. Rebased #32. Next: removing any BC layers added since then.Comment #35
wim leersDone.
Comment #36
wim leersComment #37
wim leersPer #34.
Comment #38
wim leersComment #40
wim leersNow that #3087874: [meta] Make 9.0.x branch pass testbot is done, let's do this!
Comment #41
wim leersTo start us off: a rebase of #35.
Comment #42
wim leersFixed phpcs violations.
Comment #44
wim leersComment #45
wim leers😈
--binaryComment #47
wim leersSome new tests were added since #35 in February, this removes BC layer logic from those as well.
This patch won't be green yet, but it'll be quite a bit closer.
Comment #48
wim leers😈
--binaryComment #49
jibranFYI; this will conflict with #3087644: Remove Drupal 8 updates up to and including 88** so this is pp-1 technically.
Comment #51
wim leers#49: Easy rebase for either one though! :)
Yay, #48 is down to 20 failures.
Most of the remaining failures are caused by a single removal of deprecated code, specifically:
… because the two
@RestResourceplugins in Drupal core that were still using the deprecated declaration had not yet been updated! Easy fix :)Comment #53
wim leersLowest number of failures yet! 🥳
Let's bring it even lower.
Comment #55
wim leersThe last failures are caused by
\Drupal\Component\Utility\ArgumentsResolvertreating non-object-typehinted function arguments really weirdly. In HEAD, this automatically falls back to the BC layer (also see #5), but this patch removes that. So we have to actually fix it.This should be green 🔥😬
Comment #56
wim leers🥳
🙏
Comment #57
berdirThis is technically a BC break between 8.9 and 9.0, removing constructor arguments is tricky.
To be extra careful, we could remove the type hint and check for that, basically adding a D10 deprecation. Not sure if it's worth it.
Lets fix this first in #3067762: Fix bug in \Drupal\rest\RequestHandler::createArgumentResolver and handle "Passing in arguments the legacy way is deprecated" deprecation, has tests and everything, just needs a review :)
Comment #58
berdirDon't we need an update function that now *removes* this setting, as we remove the default and (eventually)schema for it?
Otherwise any D8 site will have it in their config export still.
Comment #59
wim leers#57.2: oooh!!! I was looking for an issue about that, but couldn't find it! And I even reviewed that issue… 🙈
RTBC'd #3067762: Fix bug in \Drupal\rest\RequestHandler::createArgumentResolver and handle "Passing in arguments the legacy way is deprecated" deprecation, and marking this postponed on that issue.
#58 + #57.1: I'll dig into that once #3067762: Fix bug in \Drupal\rest\RequestHandler::createArgumentResolver and handle "Passing in arguments the legacy way is deprecated" deprecation lands.
Comment #60
wim leersNote: I'm happy to split this up into multiple patches, because this is one very big patch obviously!
Comment #61
berdirThe blocker is in, so this can be rerolled and we still need to clean up the BC setting per #58
Comment #62
wim leersYay!
Comment #63
wim leersFirst, a straight rebase (with dozens of conflicts to resolve, which was to be expected).
Comment #64
wim leersThis fixes #57.2 and #58. I think #58 also should have an update path test? (That's not yet in this patch.)
Comment #65
berdirProbably not a bad idea to have a test? We can repurpose the existing fixtures on top of the new 8.8 dumps instead and then test with that?
what about keeping the helper function? seems to be called 33 times or so and the replacement is not trivial with the timezone and correct format.
Unfortunately the name of the trait has BC in it.
Another problem is that this trait itself was not deprecated nor internal, and by removing it, we kind of break tests between 8.9 9.0 if they would use this.
Looks like we had a BC layer without explicit @trigger_error() considering that we have to fix this now? could we add that still to 8.x?
unrelated reformatting?
mabe we can actually remove this now with the delete?
this was before we added support for hook deprecation, another thing that we might want to do still in 8.8 to make it explicit and easier to detect?
I'd rather just not touch the updates here at all (except adding the new one) and rely on the existing issue to clean that up.. This will just conflict with that.
Comment #67
wim leers@trigger_error()in this hunk of\Drupal\rest\Plugin\ResourceBase::routes():BUT note that this change is fixing a bug, see #2293697-124: EntityResource POST routes all use the confusing default: use entity types' https://www.drupal.org/link-relations/create link template if available. We chose to not fix the bug in the annotation back then to avoid breaking Drupal 8 modules that were hardcoded to overwrite this particular entity type annotation key. That's why we have to change it now.
I think it makes sense to still add a
@trigger_error()in8.8.xand8.9.x..serializationbut before it shipped in a stable release, we moved it tohal. See #2758897: Move rest module's "link manager" services to serialization module + #2854830: Move rest/serialization module's "link manager" services to HAL module.)For points 2 and 5: attaching a 8.8/8.9 patch here to add those explicit deprecations.
Note: test coverage requested in #64/#65 still needs to be written.
Comment #69
wim leersComment #70
wim leersThe
9.0.xpatch in #67 was green!This fixes the
8.8.xdeprecation patch.Comment #71
berdir1. It's not important enough to fight over it, if alexpott/catch are OK with it then fine. I do think we should at least make it a D10 deprecation instead of just removing it, as again, removing it means code that works on 8.9 without deprecations will break on 9.0 and we're trying to avoid that?
Did you forget the interdiff in #67? That's a tough one to review as a whole again ;)
Comment #72
wim leers1. It was explicitly in a
BcTimestampNormalizerUnixTestTraittrait that got added in #2768651: Let TimestampItem (de)normalize to/from RFC3339 timestamps, not UNIX timestamps, for better DX, so … 😢You're right that it wasn't triggering a deprecation error like it should've, but it predated that practice. I'm also not entirely sure if it should have triggered a deprecation error, because it was specifically added to allow for BC-compatible testing. Tricky. Happy to expand the 8.8 patch for this if you know of a similar example!YES, SORRY! 🙈 Attached :)
Comment #73
wim leersDiscussed #65.1 / #67.1 / #71.1 in Slack.
We settled on the compromise to indeed keep the trait to avoid any disruption for contrib/custom modules that are using this (because it wasn't properly deprecated, this predates the deprecation process), and instead deprecate this in D9, for removal in D10. Updated https://www.drupal.org/node/2859657 accordingly.
Important note: you're not required to use
(new \DateTime())->setTimestamp(123456789)->setTimezone(new \DateTimeZone('UTC'))->format(\DateTime::RFC3339)— you can just write an RFC3339-formatted timestamp instead, i.e. just a string. You only need the complexity in case your timestamps are programmatic rather than hardcoded.In core, I preferred to opt for the more complex programmatic approach because that is more copy-pasteable. I'm happy to change that if people disagree though.
Comment #74
wim leersFix mistake in IS.
Comment #75
wim leersAdded the requested test coverage (for
rest_update_9001()).Comment #76
berdirI think you can just do assertTrue/False($rest_settings->isNew())?
Comment #77
wim leersAnd to avoid conflict with #3087644: Remove Drupal 8 updates up to and including 88** like @jibran mentioned in #49 and @Berdir in #65.6, unremoving all the update path test removals.
This means we go from
to
The reduced reduction will be handled in #3087644: Remove Drupal 8 updates up to and including 88**. Rather unsatisfying, but logical :)
Comment #78
wim leers#76: I followed a pre-existing pattern.
Comment #79
wim leersMissed one spot. (Discovered by manually comparing to #3087644: Remove Drupal 8 updates up to and including 88**'s patch.)
Final diffstat:
Comment #80
wim leers(In #79 I also discovered two bugs in #3087644 — see #3087644-91: Remove Drupal 8 updates up to and including 88**.)
Comment #83
berdir> #76: I followed a pre-existing pattern.
But you're not actually asserting anything, the before/after assert is identical? And since you don't actually set up the config in the fixture, it doesn't exist at all during the test. So the fixture needs to create that config, then assert it does exist, run updates and then make sure it doesn't anymore?
You actually delete the config, so why not assert that?
Comment #84
berdirNote: Once #3087644: Remove Drupal 8 updates up to and including 88** is in, there are a handful of checks and references to update functions (search for _update_8, e.g. \Drupal\rest\Plugin\views\display\RestExport::calculateDependencies() that we can clean up too.
Comment #85
gábor hojtsy(@berdir I think you did not mean to suggest this should be postponed on that, just wanted to highlight that this is already almost 20% of the remaining deprecations, so would be nice to get in in its own if possible sooner :)
Comment #86
wim leers#3087644: Remove Drupal 8 updates up to and including 88** landed! Rerolling 🥳
Comment #87
wim leerscore/modules/views/tests/modules/views_test_config/test_views/views.view.rest_export_with_authorization_correction.ymlbecause #3087644 didn't do that after all.Comment #88
stefdewa commentedStill had a deprecation warning for Symfony\Component\HttpKernel\Event\FilterResponseEvent. Fixed and added this to the patch, also added interdiff.txt.
Comment #89
wim leersThanks for double-checking #87 with
drupal-check, @Stefdewa! 🙏🙏🙏🥳Comment #90
berdirI'm not sure that belongs in here. drupal-check is about removing usages of deprecated API's (typically of something else), what you changed is about removing deprecated symfony event classes that are a Symfony 4 deprecation. AFAIK we already have an issue to fix that in all of core.
Comment #91
wim leers#90 Woah, of course. Good catch. I'm glad you're looking at this :)
Also, regarding #87.3:
Yay!
Comment #92
berdirthe comment is apparently now 81 characters according to dreditor. but we often leave out the . after links because that often ends up the URL otherwise. Either way, seems like something that could be fixed on commit.
I've looked through this several times and we discussed it a ton in slack as well. I'm still not 100% happy abut those convoluted date things, but Wim prefers it like this and I don't care that much.
IMHO this is ready, removes 20% or so of the remaining deprecations and unblocks the hal/serialization issues. The patch is huge but it's also very repetitive, most of it is either related to permissions and BC handling and test coverage of that or the changed date format/timezone.
Comment #93
catchIs #2893804-70: Remove rest.module BC layers still the right patch for 8.8.x/8.9.x? If so that seems fine (although could also have been spun-out to a side issue and committed with no other dependencies).
Reading the discussion about this and looking at the patch, I think I would have preferred keeping the helper method in this issue then refactoring the test coverage in a follow-up issue not specifically tied to removing the deprecations. Would mean a considerably smaller patch. However now that it's done and RTBC I don't have a strong opinion enough to push back either.
This should be fine as a post update instead of a hook_update_N() I think? Also making it a post update increases the headroom before we run into #3108658: Handling update path divergence between 11.x and 10.x in REST module.
Comment #94
wim leersYes.
I had forgotten that even existed until your reminder just now. You're right.
Comment #95
catchRight, it is, but it would also have been possible to remove the bc layer only in the method and do the rest in a follow-up. Either way fine committing as is.
Waiting for tests to run, but otherwise ready to commit this assuming they pass. Although i'm not sure the upgrade path test is going to work now - we might not need to reset the schema version, but we might need to manipulate the post updates entry.
Comment #96
wim leersI did run
\Drupal\Tests\rest\Functional\Update\RestSettingsDeletionUpdateTestlocally and it still passes :)Comment #97
catchHmm OK I think that works because of this:
i.e. we're brute-installing the module, bypassing the logic that populates already-existing post-updates (meaning they'll all run).
However that is likely to break once #3097661: No hook_update_last_removed() equivalent for post updates is committed, because we'll be on an older schema version without having run the post updates that will be removed in that patch. But since we don't 100% know what change will be required we can probably defer to that issue or a follow-up of this one.
Comment #98
wim leers#97: Fascinating. I didn't do anything special here. I just copy/pasted the pattern of how all of the REST module fixtures used to work until #3087644: Remove Drupal 8 updates up to and including 88** removed them. And AFAICT just about every fixture follows this pattern?
I'm not sure I understand what's "brute force" about this; AFAICT that's how it was intended to work — at least back when we started doing these update path tests. Perhaps there's a better pattern by now? If so, we should look into applying that to all core update path tests.EDIT: Ah now I see, it's this: — this is true, that is what happens. That has always puzzled me about our update path tests. I haven't yet seen any update path test fixture that defines which post updates have already run.
Comment #102
catch@Wim Leers yes this is all because we've never, ever removed a post update before but as soon as we have a mechanism for doing so, it'll have to happen a bit differently.
Committed 720924f and pushed to 9.0.x. Thanks!
Also committed the 8.9.x/8.8.x documentation updates with 9664d37.
Leaving the issue at 9.0.x so it doesn't induce too much panic.
Comment #103
wim leersYay!
Unpostponed #2893795: Remove serialization.module BC layers and #3034062: Remove hal.module BC layers — their patches are currently being tested by DrupalCI. If they need rerolls, they should be easy.
#102: Yep, makes perfect sense :)