Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
We've accumulated a fair number of BC layers in the serialization.module
component. In this issue, we maintain an up-to-date patch that removes all BC layers, which allows us to track how much of a difference this would make.
Comment | File | Size | Author |
---|---|---|---|
#39 | 2893795-39-interdiff.txt | 1.9 KB | Berdir |
#39 | 2893795-39.patch | 27.06 KB | Berdir |
#34 | 2893795-34-interdiff.txt | 4.98 KB | Berdir |
#34 | 2893795-34.patch | 25.81 KB | Berdir |
#30 | 2893795-30.patch | 23.69 KB | sokru |
Comments
Comment #2
Wim LeersThis removes all BC layers from
core/modules/serialization
as of July 11, 2017, commite1e216138f5311a5dd6b96b5789ab2db9ac5fd5f
.Comment #3
Wim LeersComment #4
Wim Leers1.5 year later, time for an update. Commit
c8dc57eedea3fd34de067098ca2ee36e6c459245
.Comment #5
Wim LeersComment #6
Wim LeersComment #8
Wim LeersComment #9
Wim LeersThis is blocked on #2893804: Remove rest.module BC layers.
Comment #10
kim.pepperThis is issue is postponed on #2893804: Remove rest.module BC layers that one is postponed on this?!
There seems to be a circular dependency here! 🙃
Comment #11
longwaveNo, the rest one is only waiting for #3067762: Fix bug in \Drupal\rest\RequestHandler::createArgumentResolver and handle "Passing in arguments the legacy way is deprecated" deprecation from what I can see?
Comment #12
Wim Leers#11: #10 noticed the super outdated and super wrong issue summary at #2893804: Remove rest.module BC layers 😅 Fixed that!
Comment #13
Wim LeersReroll now that #2893804: Remove rest.module BC layers is getting close.
Comment #14
Stefdewa CreditAttribution: Stefdewa at Calibrate commentedRerolled patch. Added approximate interdiff (not all changes from #13 would apply.
Setting 'Needs review' but should after review be put back to 'Postponed' because tests will fail until #2893804: Remove rest.module BC layers is merged.
Comment #15
Wim LeersObviously you found quite a few things that still needed to be removed, wonderful!
Let's use the same wording as
rest_update_9001()
in the latest patch iteration (comment 91).Marking postponed for now, but as soon as #2893804 lands, this should be re-tested and marked RTBC (and that one nit should be fixed) :) 🚢🚢
Comment #16
Wim Leers#2893804: Remove rest.module BC layers landed 5 mins ago! Testing #14…
Comment #18
BerdirI guess we also need an update path test for that, just like rest?
Comment #19
Wim LeersJust some simple updates needed and then this will be RTBC!
Comment #20
DamienMcKennaTagging as a requirement for Drupal 9.0-beta1.
Comment #21
catchIs this trait still needed?
As with the REST issue, this should be a post update.
Comment #22
Wim Leers#21:
Comment #23
sokru CreditAttribution: sokru as a volunteer commentedReroll from #14. Moved
hook_update_N
topost_update
to resolve #21.2Comment #24
Wim Leers@sokru Thanks! Could you also post the corresponding interdiff? 🙏🙂
Comment #25
Wim LeersThis comments needs to be updated as described in #15.
\Drupal\Tests\rest\Functional\Update\RestSettingsDeletionUpdateTest
.Comment #26
sokru CreditAttribution: sokru as a volunteer commentedThis should resolve #25
A bit unsure about data values provided by
core/modules/serialization/tests/fixtures/serialization-module-installed.php
.Edit: or should we extend the fixture from rest module to cover this like @Berdir mentioned in #3034062-14: Remove hal.module BC layers?
Comment #28
Wim LeersThese lines should be removed; that was only relevant for the REST module :)
EntityTestDatetimeTest
andEntityTestDateRangeTest
also still need to be updated.Comment #29
sokru CreditAttribution: sokru as a volunteer commentedCorrect path for fixture.
Comment #30
sokru CreditAttribution: sokru as a volunteer commentedStill misses
EntityTestDatetimeTest
andEntityTestDateRangeTest
.Comment #31
Wim LeersTests were added in #26, so removing that issue tag. Awaiting test results.
Comment #34
BerdirSame as in the hal issue, simplifying the update test by using the filled dump that already has the module enabled.
FWIW, bc_primitives_as_strings is really confusing as it uses TRUE to disable the BC module and then the change record talks about how to "enable" it o.0
Also fix the remaining test fail. Took me a while to figure out out (insert usual rant about generic rest test fails being hard to understand & locate), but turned out to be pretty easy, just had to remove the altered patch entity normalization that tested BC.
Comment #36
Wim LeersDown to 1 failure! Nearly there :)
I think it was the very first BC flag ever in core. Getting that done involved a string of confusions … (pun intended!)
🤗
Comment #37
Berdirthat remaining should be easy, we just need to change that test in the same way as I did the other date test already (remove the message abut the BC thing in the expected error message).
Comment #38
BerdirComment #39
BerdirFixed that last test fail.
Comment #40
Wim Leers🙈 s/serilization/serialization/
Can be fixed on commit. Or … just be left in there.
Comment #42
catchCommitted 1d1f222 and pushed to 9.0.x. Thanks!