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
Steps to reproduce:
- Enable Content Moderation
- Enable moderation on node articles
- Create a new article with draft state
- Edit draft to make it published
- Edit to make it archived
- Revert back to the second (published) revision
- Entity is still unpublished, has the default moderation state (draft)
Proposed resolution
When saving an entity create a ContentModerationState entity with the original state.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 2.44 KB | timmillwood |
#22 | 2809123-22.patch | 6.91 KB | timmillwood |
Comments
Comment #2
timmillwoodHere's a test which shows this bug.
Comment #4
timmillwoodThis patch *should* work, but blocked on #2809227: Store revision id in originalRevisionId property to use after revision id is updated.
Comment #6
timmillwoodPostponed on #2809227: Store revision id in originalRevisionId property to use after revision id is updated
Comment #7
timmillwoodNow #2809227: Store revision id in originalRevisionId property to use after revision id is updated has landed in 8.3.x we can fix this!
Comment #9
timmillwoodNeeded a check to make sure that the loaded revision ID is not the same as the revision ID.
Comment #11
timmillwoodComment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedUploading a test only patch, because it's a little puzzling why the condition right above it would ever be FALSE while the new one would be TRUE.
edit: figured out above comment after reviewing the patch in more detail.
Based on the condition right above this one I think the whole second part of this condition is superfluous. You only enter this line if getRevisionId is null.
Maybe this method could do with some more commenting.
Rest of the test uses $node->moderation_state->value =, not ->set
nit, should be full form, $previous_revision.
nit, full stop
nit, extra whitespace
Comment #13
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCould this implementation also work?
Comment #15
timmillwood@Sam152 - I quite like your solution, much simpler. I do think the whole getModerationState() method could do with a number of comments to explain how and why it all works because it takes a for moments to understand all the logic in there.
Comment #16
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAgreed. NW based on that.
Comment #17
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdded some comments and addressed feedback in #12. The extra methods are a bit more self documenting, add some extra comments and reduce the complexity of the big getModerationStateId method.
Comment #18
timmillwoodLittle code exists in #17 from my patch, so guess I am entitled to RTBC.
Looks good, fixes the bug, tidier, easier to understand, all round better.
Comment #19
alexpottLet's not have two additional methods here - I think this is unnecessary. You're always going to want the correct language. I think all we need to do is merge
loadContentModerationRevision
andloadCorrespondingContentModerationStateLanguage
together and just call itloadContentModerationState
.Comment #20
timmillwoodConsolidation of methods as suggested in #19.
Comment #21
alexpottThe early return here is fine but lets capitalise null as per standards.
Let's turn this logic around to avoid the early return. Basically as this logic was before. And then the final line can be
return $content_moderation_state;
Comment #22
timmillwoodDone #21.
Comment #23
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedinterdiff LGTM
Comment #24
alexpottCommitted 6b34291 and pushed to 8.3.x. Thanks!
Comment #27
timmillwood#2846830: Add changelog for Drupal 8.3.0