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.
Steps to reproduce:
- Create a moderate entity
- Delete it
Expected result: all records in the content_moderation_state_*
tables referencing the entity are gone
Actual result: they still are happily sitting there :)
Marking this as a "WI critical" as this can introduce data integrity issues. Feel free to demote if this is not a concern.
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff-39-43.txt | 1.18 KB | evilehk |
#43 | cm-delete-2893888-43.patch | 14.79 KB | evilehk |
#40 | cm-delete-2893888-39.patch | 14.6 KB | plach |
Comments
Comment #2
plachComment #3
plachAnd here's a test-only patch
Comment #4
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNice find!
I'm pretty sure we need to do the same for
hook_entity_translation_delete()
andhook_entity_revision_delete()
:)Comment #5
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis looks great, the refactor into loadContentModerationStateEntity is a nice clean up.
Do we need this in the middle of this test, any chance we can use the same data provider and spit it out? It's already pretty big.
Comment #6
plachSorry for the delay but I got completely derailed by trying to (x)debug PHPUnit tests via PHP Storm, which for some reason was no longer working.
This should address #4 and #5, however it still has a couple of known test failures that I was not sure how to address. I can easily fix them by adding a flag to the new
ContentModerationState::loadFromEntity()
helper method to retrieve the CMS entity revision matching the current entity revision only when explicitly specified, however that does not feel right to me.In particular, the fact
ViewsDataIntegrationTest::testContentModerationStateBaseJoin()
is now failing, but it has a @todo explicitly stating the expected result looks weird, makes me think there may actually something wrong in those tests.It would be great if you guys could have a closer look to them and figure out whether I'm doing something wrong and the current behavior is the expected one, or whether this unintentionally uncovered an unrelated bug.
Comment #7
plachAdded an assertion and renamed a few variables
Comment #8
plachComment #9
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis looks like an awesome cleanup in addition to the bug fix. I'll wait for the test results before diving deeper into the problems mentioned in #6, but here's a review in the meantime:
How about
loadFromModeratedEntity()
?Since we know that all these hook bridges are only working with content entities, why don't we type hint that directly?
I would inline the assignments in the condition, but that's just personal preference, feel free to ignore it :)
Comment #12
plach1: Done
2: Well, it was mainly for consistency: most entity hooks (including
hook_entity_delete()
) are called also for config entities, so the system would throw a recoverable fatal if we usedContentEntityInterface
for those. It would make sense for revisions and translations but it would be inconsistent, aside from the fact that also those hooks are documented as passing a genericEntityInterface
, so that could somehow happen in contrib.3: ignored :)
Leaving at NW due to the test failures.
Comment #13
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI've looked into that @todo before (#2859455: Remove superfluous @todos from content moderation). The ContentModerationState entity makes no attempt to match the default-ness of the entity being moderated. The latest ContentModerationState entity will always be the default. I believe this is by design or hasn't been considered before.
Possibly just need to remove the $default_revision check in \Drupal\content_moderation\Entity\ContentModerationState::loadFromModeratedEntity? Something like the following:
Note loading the matching translation as well.
Comment #14
plach@Sam152:
I tried that patch but it's triggering the generation of multiple CMS entities related to the same moderated entity, which is a bit weird at first sight, not sure whether that's expected. As mentioned previously, the attached patch fixes those failures, but I'm not sure it's correct.
Comment #15
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedRemoving the flag and fixing with getLoadedRevisionId(). Seems to do the trick.
Comment #16
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #17
plachNice! I'd say we're good here, RTBC on my end.
Comment #18
timmillwoodSorry, a bit nitpicky. Otherwise, looks fine.
We could do an early return here. Then we don't need to set $result to NULL, and don't need to return $result. Also, $result isn't a very clear variable name. Without reading the docblock I'd expect a DB query result.
Comment #19
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHow about the following tidy-up? loadRevision already returns NULL, so no need to check that exists.
Comment #20
timmillwoodThat looks like a great tidy up! 8 lines removed!
Comment #21
plachThe check on $ids was needed, moreover there was this:
Wrong PHP doc, copy/paste--
Here's an updated version. I'd prefer to avoid early returns, they make code harder to follow. Uploading also a "test-only" patch.
Comment #22
plachAnd the correct missing interdiff for the PHP doc, disregard the previous one.
Comment #23
timmillwoodI'm not going to be too precious either way, and good catch on the PHP doc.
Comment #24
plachAaand simpler :)
Comment #25
plachOops, sorry, crosspost. Back to RTBC
Comment #26
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented+1 LGTM
Comment #27
timmillwoodPerfect!
Comment #30
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI looked over this patch as well and I could only find two small cosmetic issues. Fixed them quickly with the attached interdiff :)
Comment #31
catchThis should have an accessCheck(FALSE). In practice I can't ever see it being necessary, but bad if it does and it's a common mistake with nodes that are affected by node access modules.
Is this still a @todo and if so does it need an issue?
Comment #32
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThat @todo is probably not relevant anymore now that #2844594: Default workflow states and transitions is in.
Comment #33
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedFixing feedback from #31, I think these changes are small enough to go back to RTBC.
Comment #34
timmillwoodLGTM!
Comment #35
plachRTBC +1
Comment #37
catchSorry another one I didn't notice first look through and these are all nits... isn't this identical to $content_moderation_state->deleteRevision($content_moderation_state->getRevisionId());?
Comment #38
plachThere is no
::deleteRevision()
method on the entity itself AFAICTComment #39
timmillwood@plach it looks like you're correct.
Comment #40
plachfixed typo
Comment #41
timmillwoodAs this is already RTBC it should be fine for 8.4.x
Comment #42
catchYes you're right, not sure where I came up with that non-existent method from, but looks good from here then.
Comment #43
evilehk CreditAttribution: evilehk at Breakthrough Technologies, LLC for Breakthrough Technologies, LLC commentedRebase of patch from #40 with latest 8.4.x branch.
Comment #44
timmillwoodComment #45
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!