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
Currently LB has a @todo, which when resolved would introduce dataloss and integrity issues with content revisions:
if ($entity instanceof RevisionableInterface) {
// If the parent entity will have a new revision create a new revision
// of the block.
// @todo Currently revisions are never created for the parent entity.
// This will be fixed in https://www.drupal.org/node/2937199.
// To work around this always make a revision when the parent entity
// is an instance of RevisionableInterface. After the issue is fixed
// only create a new revision if '$entity->isNewRevision()'.
$new_revision = TRUE;
}
Work is already underway to fix that in #2937199: Track Layout override revisions on entities which support revisioning. The patch that resolves the @todo goes completely green, despite the actual solution being problematic.
Proposed resolution
Resolve the @todo by removing it and proving with tests why it's actually a required component of the current revisioning model.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff-3053887-28-30.txt | 1020 bytes | fenstrat |
#30 | 3053887-30.patch | 7.02 KB | fenstrat |
#4 | 3053887-4-FAIL.patch | 4.37 KB | Sam152 |
Comments
Comment #2
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #3
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #4
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHere is an integration test. All it does in LB core is make the change proposed in this issue and rename some of the params to better indicate what they are actually doing.
The fail patch is the same test, but with the @todo resolved in as proposed by the todo and the implementation in #2937199: Track Layout override revisions on entities which support revisioning.
I would like to resolve this and agree on the model of the revisions before fixing #3053881: Reverting entity revisions that contain custom blocks erroneously triggers EntityChangedConstraint, since the proposed resolution there depends on these semantics to function, so I have made this test coverage a blocker for that issue.
It may also be worth some lower level integration testing to assert revision IDs instead of content as well.
Comment #6
tedbowThis looks good. I think I realized we couldn't not make new revision at some point but forgot to remove this.
the only case where we could not make a new revision is when there aren't any previous revisions of the layout entity. Actually I guess if there no revisions where the layout field is not null. Not sure if make sense to check that. But if you have an entity that supports revision but you don't ever create new revisions then if you make 100 new changes to the layout(no revisions) we still make 100 revisions of the inline block. But the 99 revisions wouldn't be reference anywhere.
Comment #7
johnwebdev CreditAttribution: johnwebdev commentedCorrect, but to assert that we would need to look at all revisions , to ensure none of them has that revision ID, otherwise we would introduce a data loss.
Comment #8
tedbowWell if we only to cover the case where there NO revisions with any layout data we just have to use and entity query to make sure there are no other revision where the layout field is not NULL. We don't actually have to look at the field value in that case.
Comment #9
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedDo you think that belongs in a follow-up, since it's adding new behavior?
Also, do you think it's worth trying to formalise this a bit more? Right now "modified" depends on if a block was originally serialized or not. I also don't know if there is any coverage around the converse of this, that is what happens when a block is skipped because it wasn't originally serialized.
Comment #10
tedbowSure. Created #3054886: Do not create new revisions for inline blocks when there are no previous revisions
We could add an assertion to
\Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockTest::testInlineBlocksRevisioning()
that when a block is not edited(it won't be serialized) then a new revision is not created.Comment #11
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI've added the assertions to
testInlineBlocksRevisioningIntegrity
, since this tests the scenario of updating a layout with multiple blocks, where only one has an actual content change.Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHey @tedbow, did you have any other feedback or concerns with the coverage and approach here? Keen to move this forward as it's blocking the revert issue which is blocking some significant integrity and usability on a project I'm working on.
Comment #14
tim.plunkettBump
Comment #15
lpeabody CreditAttribution: lpeabody commentedBump again. Not being able to revert is causing some major pains for clients.
Comment #16
Poindexterous CreditAttribution: Poindexterous commentedBump, the revert issue is effecting me also.
Comment #18
tim.plunkettAssigning to @tedbow per #12
Comment #19
tedbow@Sam152 sorry it took me so long to get to this.
Just a couple of
I think this comment should be moved inside the if statement. Otherwise it seems to ignore the case where
$block
is set because of the duplicate block logic above this.For this test the user doesn't actually edit the node only the layout. So they don't need these permission.
Wonder if this would be more clear if we used 'access content' and 'view all revisions' instead.
Comment #20
Deepak Goyal CreditAttribution: Deepak Goyal at Srijan | A Material+ Company for Drupal India Association commentedComment #21
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedPlease don't assign issues unless you intend on working on them.
Thanks for the review @tedbow, those changes seem reasonable.
Comment #22
tedbowSorry my dreditor comment was off we don't need 'bypass node access' can be replace with 'access content'
Otherwise looks good 👍
Comment #23
tedbowComment #24
Lal_Comment #25
Lal_sorry wrong interdiff
Comment #26
tedbowThis looks good. Thanks for the patch @Lal_
Thanks for all the work @Sam152!
Comment #27
larowlancan we just inline this while we're here?
ie
This change of variable name looks out of scope - is there a reason we're doing so here? Can we just improve the doc-block to make it clear that it's 'if modified'?
same here
We assert this twice without any changes? Is one a duplicate?
Comment #28
acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commentedFixes #27, agreed that renaming the vars is not necessary.
Comment #29
tedbow@acbramley thanks for the patch.
re #28
$new_revision_if_modified
actually makes the comment above it confusing. The specifically refers to the what we are sending to the vrar we will send to\Drupal\layout_builder\InlineBlockEntityOperations::saveInlineBlockComponent()
for the$new_revision
argument.but now the comment is before the loop and it is not clear what is referring to.
Also won't this be less performant anyways to do the
$entity instanceof RevisionableInterface
test multiple times instead of just once. We don't know how many will be returned from$this->getInlineBlockComponents($sections)
But performance is probably a small issue. I think having it separate for the comment is more important.
If others don't think the comment is now confusing thats fine.
Other than that looks good
Comment #30
fenstratFixes #29. Agreed with @tedbow that inlining that argument doesn't make sense given the comment above specifically relates to it, and it avoids reevaluating it inside the foreach.
Comment #31
tedbow@fenstrat thanks for the update. RTBC🎉
Comment #33
larowlanCommitted 109fe60 and pushed to 9.1.x. Thanks!
Thanks all for seeing this one through