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
This manifested itself here: #2848298: Paragraphs entity base table left without a revision ID after replication. When duplicating a non-default revision, this leaves content data tables in an inconsistent state as the row in the entity base tables has NULL as the revision reference.
Proposed resolution
As suggested by @berdir isDefaultRevision should should also be sensitive to isNew, ensuring duplicated entities are always the default revision.
Remaining tasks
Validate, test and patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#11 | 2850022-duplicated-non-default-revisions-11.patch | 4.19 KB | Sam152 |
#11 | 2850022-duplicated-non-default-revisions-11_TEST_ONLY.patch | 3.55 KB | Sam152 |
#11 | interdiff.txt | 1.06 KB | Sam152 |
#8 | interdiff.txt | 2.25 KB | Sam152 |
#8 | 2850022-duplicated-non-default-revisions-8.patch | 3.84 KB | Sam152 |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 at PreviousNext commentedComment #3
Sam152 CreditAttribution: Sam152 at PreviousNext commentedCredit to @berdir for the fix itself.
Comment #6
Berdirwe should add a comment
I thought we already have a dedicated test for that, but apparently not.
The test fail should be easy to fix, we juts need to mock isNew() in there.
Comment #7
BerdirActually, since this is isn't replicate specific, we might want to add unit test coverage to that test there as well, to test the different cases.
Comment #8
Sam152 CreditAttribution: Sam152 at PreviousNext commentedTried doing this so we weren't mocking methods on the system under test, but not luck, it required a lot of extra entity key/field definition/language mocking code and caused conflicts where other test methods had already mocked specific behaviors for those methods (like the hasKey method in testIsNewRevision). I can post progress on that, but this approach seems okay.
We could probably drop the kernel test entirely, but maybe it can start as the foundation for any other dedicated tests.
Comment #10
BerdirWe should expand this to ensure that we can save and load the new entity again. To be sure that we don't introduce a different regression related to cloning a non-default revision in the future.
Everything else looks fine to me.
Comment #11
Sam152 CreditAttribution: Sam152 at PreviousNext commentedExpanded coverage as requested.
Comment #13
BerdirLooks good to me now.
Disclaimer: The actual bug fix was suggested by me, but @Sam152 wrote the patch and the tests.
Comment #16
catchCommitted/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!