Problem/Motivation
Drupal\content_moderation\EntityOperations::updateOrCreateFromEntity() has this code in it:
// If a new revision of the content has been created, add a new content
// moderation state revision.
$content_moderation_state->setNewRevision(TRUE);
Which is great, except it results in the newly created moderation state revision being created as a default revision even if the moderated entity is being saved as a Draft. I don't know if this has problems in and of itself, but #2860097: Ensure that content translations can be moderated independently requires the moderation state entity's pending/default status to match the moderated entity's one, and it's possible that this mismatch is also all of or part of the problem that leads to #2933099: "Default Revision cannot be deleted." But it's not the default revision....
To reproduce the bug:
- Save an Article as Published. Look at the `content_moderation_state` db table and notice that its revision ID is the latest one from `content_moderation_state_revision`.
- Edit the Article and save it as Draft. Look at the `content_moderation_state_revision` db table to confirm a new revision got made. Look at the `content_moderation_state` table. Expected behavior: it should still be the one from step 1, if we want the new revision treated as a pending revision. Actual behavior: It's the latest one, which resulting as the new moderation state revision being treated as a default revision despite the corresponding Article revision still being a pending revision.
Proposed resolution
See patch.
Remaining tasks
Implement hook_update_N() to update existing moderation state entities/revisions to match their host entity's/revision's pending/default status.
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#25 | cm-ms_default_revision-2941736-25.patch | 119.93 KB | plach |
Comments
Comment #2
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #4
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCopying the
ViewsDataIntegrationTest
changes from #2860097: Ensure that content translations can be moderated independently.Comment #5
effulgentsia CreditAttribution: effulgentsia at Acquia commentedYay that this patch fixes a WTF that already tripped up whoever wrote that :)
Seems like we're now missing test coverage for some case where there's 'draft' as a value?
Comment #6
plachWorking on the update logic
Comment #7
plachThis implements the upgrade path, we need test coverage for both the upgrade and the fix itself. I'll work on that over the weekend if none beats me to it.
Comment #8
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #10
plachHere's a test for the main functionality, we are only missing the update test now.
Comment #11
plachComment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI had a go at creating some db fixtures based on
drupal-8.bare.standard.php.gz
which would install and update core, installcontent_moderation
and any associated content at the8.4.0
schema, so there is a general foundation for update path tests at the point which content moderation hit stable. This approach doesn't work very well, because there is a mixture of rows from the 8.0.0 schema and 8.4.0 schema. New columns introduced between 8.0 and 8.4 are mismatched before you callrunUpdates
in the test. It's also probably a bad idea to try and create tests that aren't based on completely concrete snapshots of the schemas we are trying to test against.I know there is always hesitation to introduce new full gzipped snapshots, but I think based on cm being new in 8.4.0, we need it. For that reason I think it makes sense to create some new fixtures for this:
drupal-8.4.0.bare.standard.php.gz
: Installation of standard at 8.4.0, can be used for other update path test with things introduced for the first time in 8.4.drupal-8.4.0-content_moderation_installed.php
: Installing content moderation on top of 8.4.0, can be used for other CM update test.2941736-content.php
: Content created at 8.4.0 required to test this issue.Happy to have a squiz at doing this, or if you have a plan for it @plach, I can leave you to it. I've attached an interdiff on top of #10 which is a rough failed attempt at using
drupal-8.bare.standard.php.gz
.Comment #13
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedHere is an implementation of the fixtures proposed in #12 and an associated test. If the test cases change, I've attached the script used to generate content for the third fixture file referenced by the test. The first two can be imported, the script can be run and the resulting delta is just the test cases required for the test.
Apologies if this is stepping on the toes of any work in progress or plans you had for the issue, if it is, feel free to go in another direction.
Comment #14
plachLooks great, however while testing it I realized there was a problem with the update code. Working on a fix.
Comment #15
plachThis seems to work nicely now. I'm attaching also a before/after SQL snapshot of the database generated by Sam, for people wishing to review its content.
Comment #16
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI split out #2942073: Add a "bare.standard.php.gz" test fixture that starts from Drupal 8.4.0., RTBC'd it, and pinged several of the other committers about it to get their take on it.
Comment #17
plach#2942073: Add a "bare.standard.php.gz" test fixture that starts from Drupal 8.4.0. went in, here's a reroll
Comment #18
plachAttaching also a code-only patch, for reviewers' convenience.
Comment #19
timmillwoodWe don't generally abbreviate things, and cms means more than Content Moderation State.
When happens if the entity type doesn't have a bundle key?
Maybe we should set the alias to cmsfr, because that's the revision data table?
It might be good to test other entity types too? Such as entity_test_no_bundle.
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI think this update function is unnecessarily complex.
First, it only deals with SQL storages without a check whether the moderated entity is using a SQL storage or not.
Second, I don't really understand why it has to be hardcoded to SQL queries at all. Since it is a post_update function, can't we simply do something like this:
Is there any reason to use the new
createRevision()
method instead of the previoussetNewRevision(TRUE)
call? Do we need to merge the default revision with the active translation of the CMS entity?Comment #21
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdding an issue for a fixture to enable entity_test for scenarios like #19.4.
Comment #22
plachThis should address the reviews above.
Comment #23
plach@amateescu, #20.2:
Well, aside from the fact that that's the new recommended way to instantiate new revisions (we may even add a
hook_entity_revision_create()
invocation to that method), yes, we will be going to need the multilingual merge logic in #2860097: Ensure that content translations can be moderated independently.Comment #24
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNice, this looks much cleaner now! Just a few nitpicks from me left:
Needs a comma between "entity" and "identify" :)
We don't do direct table queries anymore, so I think we can remove the "in the "content_moderation_state" entity base table" part of this comment.
Are we sure this doesn't throw a notice the first the update function runs?
We could use
$settings['entity_update_batch_size']
for the default limit, 10 seems to be a very low limit.Comment #25
plach@amateescu:
Yep, the & operator initializes the key as NULL if it's missing.
Comment #26
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedCool, I think this is ready now :)
Comment #27
effulgentsia CreditAttribution: effulgentsia at Acquia commentedLooks great. Adding reviewer credit.
Comment #30
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.6.x and 8.5.x.
Comment #31
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCome to think of it, I think this could use a change record, just in case anyone was relying on the prior behavior of the content_moderation_state entity's default revision always being the latest one regardless of the state of the moderated entity.
Also, while #19.2 got fixed, it doesn't look like the entity-type-with-no-bundle case was added to the test fixture. Could we have a followup to do so, just to make sure that code path has test coverage?
Comment #32
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNote that content_moderation was still experimental in 8.4. So that makes me less clear on whether a change record for this is needed or not. What do others think?
Comment #33
plach@effulgentsia:
We had opened #2942196: Create a database fixture to enable entity_test on top of drupal-8.4.0.bare.standard., but it seems
entity_test
is not a good candidate for a test fixture.I'm not sure about the CR: aside from the fact that CM was experimental so far, the CMS entity type is internal and not supposed to be interacted with directly. Of course there is a behavior change, but I'm not sure whether this change is perceivable in any other way then a bug fix. I'd like to read the CM maintainers' opinions on this.
Comment #34
plachTentatively marking this as fixed. Feel free to reopen if you disagree.