Problem/Motivation
For a long time content_moderation
has always created a new revision when saving a moderated entity and controlled the publishing and defaultness of that revision according to the configured moderation workflow.
There are some cases where a moderated entity might require non-content related metadata to be updated, content to be synced into a particular revision from some other source (such as the lingotek module syncing translation information into a particular entity translation revision) or updated via a workspaces deployment in the future: #3037136: Make Workspaces and Content Moderation work together.
Proposed resolution
It would be a significant change in the API to switch off new revisions by default for content moderation, but we can leverage SynchronizableInterface
to provide an API to opt out of specifically the semantics of:
- Forcing a new revision every save.
- Adjusting the default status of the revision being saved.
Specifically while syncing, changes to the moderation state itself:
- Will still be reflected.
- Will still update the 'publishing' status of the revision according to the given workflow.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
New revisions are created every time an entity or revision is updated, if that entity is being moderated with content moderation. Previously there was no way to opt out of this. By using SynchronizableInterface
, users can now mark an entity as 'syncing' to indicate changes are being made to the entity that do not reflect a typical content/editorial field based change and thus should not be subject to the entity lifecycle rules content moderation enforces. If you are using SynchronizableInterface
in custom code for content entities and also depend on content moderation forcing the creation of new revisions, you may need to update your code to manually call $entity->setNewRevision(TRUE).
Comment | File | Size | Author |
---|---|---|---|
#75 | hook-weight-test.txt | 3.95 KB | Sam152 |
#48 | 2803717-48.patch | 9.39 KB | Sam152 |
#48 | interdiff.txt | 604 bytes | Sam152 |
#46 | 2803717-46.patch | 9.38 KB | Sam152 |
#46 | interdiff.txt | 745 bytes | Sam152 |
Comments
Comment #2
dawehnerComment #3
dawehnerComment #5
yogeshmpawarI have rerolled the patch to be applied for version 8.3.x
Comment #7
timmillwoodLooks like the tests don't agree we can remove this.
Comment #8
dawehnerWell, the right way is to always configure the node type correctly. Once this is done, it should work, without this custom code.
Comment #9
timmillwoodI've updated the patch to only enforce a new revision if the bundle is not configured to.
Comment #10
dawehnerthe entity type here is on the nodeType, but $entity is a node, so this doesn't work.
To be honest though I don't believe this is the right angle to solve it. We need some way to detect whether anything has explicitly changed this value. In case they did, we don't want to change the opinion.
Comment #12
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedOn to the point of detecting the change, could it be done purely at a UI level? We already have \Drupal\content_moderation\Entity\Handler\NodeModerationHandler::enforceRevisionsEntityFormAlter which ensures the checkbox is checked, maybe that's enough and we can assume any kind of programatic changes to entities will result in the new revision flag being respected?
'new_revision' and 'revision' would need to go in the entity handlers for the current approach, so NW anyway.
Comment #13
kmajzlik CreditAttribution: kmajzlik at Ciklum Western Europe for BurdaForward commentedWhy should be new revisions enforced? What if i would like to fix just small typo as an editor? I do not want to have 20 drafts created in 5 minutes by single editor.
Still searching proper way to get Content Moderation working like this image is working in Workbench Moderation in D7: https://www.drupal.org/files/images/workbench-moderation-screenshot.png
Patch in #5 simply works for me fine (even with those failing tests) together with small hook_form_node_form_alter() which removes #disabled from revision item in form.
Comment #14
kmajzlik CreditAttribution: kmajzlik at Ciklum Western Europe for BurdaForward commentedAnd the same thing is about submiting moderation form - i really do not want to create new revision. Workflow != content.
Comment #15
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@Sam152:
I don't think we can assume that because, without the
setNewRevision()
call that's being questioned by this issue, calling$entity->save()
without also callingsetNewRevision(TRUE)
before will happily overwrite the existing revision without creating a new one.@dawehner:
That would imply making
\Drupal\Core\Entity\ContentEntityBase::$newRevision
a three-state property (NULL, FALSE, TRUE) with NULL as a default and makingsetNewRevision()
fail if$newRevision
is not NULL, which I think would be an API change.I have another suggestion though:
\Drupal\Core\Entity\ContentEntityBase::setNewRevision()
is kind of a one-way street in HEAD, it only handles the case when changing the value from FALSE to TRUE by removing the value of therevision_id
field and preparing the translations for a new revision.So how about making it also handle the case when we want to go from TRUE to FALSE, by setting back the revision_id to whatever value it had before (probably using
getLoadedRevisionId()
) and reverting the translation changes?That way, the lingotek module for example could make sure that its
preSave()
code runs after Content Moderation so it will have the final decision whether a new revision should be created or not. This, however, has the disadvantage that the module which decides to not save a new revision also has to be sure (as in.. really sure) that there are no fields changed other than its 'metadata' fields.Comment #16
BerdirWe have #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously for doing something like that.
Comment #19
timmillwoodNow that #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously is in, we should re-ignite this issue.
Comment #20
Sutharsan CreditAttribution: Sutharsan commentedSince #9 the solution will probably to a different direction. But I took a spin with #9 in 8.6.x and got the solution below. Perhaps it is useful to others.
Comment #21
Chris Gillis CreditAttribution: Chris Gillis commentedI also encountered this issue. I am saving entities programatically, explicitly setting revisions on/off where necessary... however even when attempting to save without a new revision, it would always force a new revision.
#5 applied cleanly to Drupal 8.5.3 and fixed my issue.
As this breaks Drupal core API (
$entity->setNewRevision(FALSE)
fails silently) I think this can be considered a major bug.Comment #23
matthiasm11 CreditAttribution: matthiasm11 at Randstad Digital commentedPatch #9 will break a programmatically saved entity with
$entity->setNewRevision(FALSE);
.I agree with #21 that patch #5 is the way to go.
Comment #24
Berdir#5 will only work for node types where moderation is not enabled and if nothing else, we need tests.
I also think that new revisions shouldn't be forced on the API level, especially not when explicitly calling setNewRevision(FALSE) and not just for those that don't have that checkbox set aka don't use content_moderation on a given bundle/node type.
Comment #26
maximpodorov CreditAttribution: maximpodorov commentedOne more critical problem with such forcing: when we cancel a user with assigning the content to anonymous user, node_mass_update() functions loads all revisions and updates them. It's not expected that new revisions can be created by this process, but actually they are created for moderated content breaking the data in the database (e.g. revision_translation_affected field can become incorrect after that). So I suggest to treat the issue as critical.
Comment #27
BerdirLets see what happens now if we try #5 again.
Comment #28
BerdirComment #30
BerdirSo those test fails are quite interesting, as we have all sorts of things no longer getting new revisions as the tests expect: forms, API tests, json_api tests, especially the form part seems interesting.
Seems like what we need first is a definition when exactly a new revision shouldn't happen and when not. The form test fails indicate that even with content moderation enabled, the UI doesn't always result in a new revision, seems like that definitely should happen.
One option would be to still default to always doing a new revision, but respecting an explicit FALSE, that e.g. node_mass_update() would need to set.
It's again really hard to make out the line between bugfix and API change. I feel like enforcing it like this *is* an API change (mass update nodes works fine => enable CM => mass update is broken), but it's also been like this forever when the module is enabled, so anyone who relies on it now could argue it is a BC break if we no longer enforce it.
Comment #31
catchWhile this sounds very broken, I'd expect node_mass_update() to create a new revision in this case. The main problem here is that in most circumstances we wouldn't allow a new default creation to be created via the UI if the node is in moderation.
Comment #32
maximpodorov CreditAttribution: maximpodorov commentedFor that particular case what is needed is the rough data change of all revisions, not the creation of a new revision, since we don't make changes which can be compared with some previous state in some past revision.
Comment #33
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI think @Berdir is right re: the API concerns, forcing a new revision feels pretty fundamental to moderated entities at this point. Respecting an explicit
FALSE
might be viable, but @amateescu points out some problems with that in #15.What about using
\Drupal\Core\Entity\SynchronizableInterface
instead, for something a little more explicit? Content moderation could dial back some of it's aggressive revision semantics if an entity is flagged as syncing. The name kind of sounds like it fits the lingotek andnode_mass_update
use case.It does however sound like the real bug that should be addressed is:
Comment #34
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSeeing what the impact of #33 is.
Comment #35
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNote that #2544790: ContentEntityBase::setNewRevision(FALSE) is broken if ::setNewRevision(TRUE) was called previously was committed since then, so the better fix for lingotek at this point would be to ensure that it has a
hook_entity_presave
implementation that runs after CM so it can overrule the creation of a new revision.However, the
node_mass_update()
case is a different problem because it doesn't handle only "metadata" field updates like lingotek, so I think it would be better to look into it separately.As for the patch in #34, I think it makes very much sense to do that (as proposed in #3037136-2: Make Workspaces and Content Moderation work together as well), but not really as a fix for the problems mentioned above.
Comment #36
maximpodorov CreditAttribution: maximpodorov commentedBTW, complex migrations are much better with the patch #27. No more useless multiple revisions, just one revision of every node! :)
Comment #37
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI think it probably makes sense for #26 to be spun out into it's own issue.
Comment #38
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI had another look and wrote some tests for this. I tried to capture and test what would happen while syncing and updating the moderation state: what happens to the state, publishing status and defaultness. The simplest approach that makes that most sense IMO is:
The last point is the pertinent one IMO. Setting an existing revision to be non-default, while not creating a new revision will not actually do anything at the storage level. Another revision must actually be promoted to be the default. Going through prior revisions, calculating if it was previously the default and reverting decision I think is complex, risky and may not necessarily be what is expected.
The downside of that approach is you could sync to a non-default status, for a default revision, but given this is an API level construct, I think it's an appropriate trade-off.
Comment #39
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdjusting the issue summary to the approach in #38.
Comment #40
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #41
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdjusting the comment and control flow to be a little more clear.
Comment #42
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #43
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedJust thought of another really good reason to exclude all changes to defaultness while syncing: syncing changes to previous revisions. Codifying that in a test.
Edit: the second uploaded interdiff is the correct one.
Comment #44
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #46
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedWrong test base.
Comment #48
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedComment #49
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedJust read through #38 a few times and I fully agree with it. Not creating a new revision or changing the defaultness while syncing is fairly obvious. I pondered a bit about allowing moderation state changes (accompanied by publishing state changes) and in the end I think that's very much needed, otherwise changing the moderation state during a sync process would bypass the moderation workflow of the entity.
The patch looks great as well!
Comment #50
maximpodorov CreditAttribution: maximpodorov commentedSo this mean in the custom code it's required to set syncing flag:
every time we need to save an entity:
Otherwise the new revision will be created.
Comment #51
BerdirDidn't review the patch yet, just a quick comment that the the sync flag is also being discussed for #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating) and there are several comments there that argue that using this generic flag isn't very intuitive.
This issue now only adds the API and a test for it, but I think we'll also need to review which cases in core need to use this flag now, like the mentioned revision author update process. That could be a follow-up, but then we'd need to create it in advance I think. And looking at the use cases might be useful before committing to a specific API/flag, as that might also help us decide if that makes sense in those contexts? Unlike the ChangedItem issue, the cases we have here need to work with and without content_moderation, so that's an important argument for using this generic flag.
A change record might also be useful?
Comment #52
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@maximpodorov, I think it depends on a case-by-case basis. For example, migrations should probably use
setSyncing(TRUE)
by default, but, as @catch pointed out in #31,node_mass_update()
probably shouldn't.Comment #53
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks for the review @amateescu and @Berdir. Happy to add a change record.
Re: follow-ups, adding the flag to migration updates might be a good way to see this working in a real context.
Comment #54
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedCreated the change record here: https://www.drupal.org/node/3052114
Comment #55
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedLogged one of the discussed follow-ups here: #3052115: Mark an entity as 'syncing' during a migration update.
Comment #56
maximpodorov CreditAttribution: maximpodorov commentedI don't think the content moderation logic must dictate all over the core and contrib and custom modules what is entity syncing and what is not. We can come to the absurd by this way:
"every entity update is syncing if it's not the result of human action of entity editing (via entity form or mass actions on the content dashboard)".
Isn't it better just to return to simple solution #3 and use $entity->setNewRevision(TRUE) in the places where it's necessary for the content moderation logic?
Comment #57
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIt doesn't, it just reacts to the fact that an entity save process declared itself as 'syncing'.
The 'simple solution' doesn't work. As soon as you enable content moderation for an entity bundle, the moderation workflow must be enforced at the (entity save) API level, otherwise we risk bypassing it when using regular entity API calls.
Comment #58
maximpodorov CreditAttribution: maximpodorov commentedSo (as I mentioned in #50) we have to review ALL the code which saves entities and add setSyncing() calls if necessary. Isn't it a very high price?
Comment #59
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI suppose it's up to the caller to decide if a particular scenario meets the definition of an entity that is 'syncing'. So far with content and config entities, those are: deploying a workspace and importing configuration into a site.
Based on the original report that triggered this issue, I feel like pulling in translation data from an external service such as lingotek and adding it to individual revisions and translations also comfortably meets the definition of 'syncing', so the whole fix seems kind of appropriate to me.
Comment #60
larowlanI agree with Berdir on this one, this API is very generic.
Should we consider expanding setSyncing to work with bitwise flags instead of booleans?
Then we can move away from it being a binary flag?
Comment #61
larowlanI think there is more discussion to be had here, as seen in #2329253: Allow the ChangedItem to skip updating the entity's "changed" timestamp when synchronizing (f.e. when migrating)
Also, I'm not sure this is critical - which of the following does this cause?
Happy to be wrong on that
Comment #62
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI would rather make sure that
SynchronizableInterface
was documented thoroughly enough so it was:Then both module developers and callers have some more concrete guidelines in which to operate. I think adding extra flags to the method is essentially just creating N flags, instead of meaningfully defining what the current one we have is. The net result IMO would be more complex and less useful.
IMO, the current 'syncing' status for content entities is shaping up into a flag to indicate that a save is happening under conditions which are outside of a typical user initiated content update and that as much as possible, side effects from that save should be limited. Examples might be:
After defining some of those scenarios it becomes easier to rationalise eliminating some of the side effects that have been discussed (CM revision handling + changed timestamp).
Just my 2c, interested to hear what others think.
Comment #63
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedVery much agreed with all of #62 :)
Should we update the documentation of
SynchronizableInterface
in this issue or open a new one and postpone this on it?Comment #64
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSpun off #3057483: [PP-2] Better describe how SynchronizableInterface should be used for content entities for consideration.
Comment #65
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAnother user was reporting #26 in another issue, so I have spun out #3062900: The combination of content moderation, translations and cancelling user accounts is broken..
Comment #66
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks @Sam152, for opening #3057483: [PP-2] Better describe how SynchronizableInterface should be used for content entities! I think that can be discussed as a followup, since this patch is preventing progress in #3037136: Make Workspaces and Content Moderation work together, which is a stable blocker for Workspaces.
In think @Berdir agreed with going forward with the current approach in #51 (last sentence of the second paragraph), and we also have the requested CR.
Also, the suggestion from #60 (even if I don't necessarily agree with it, as mentioned in #63), could also be implemented in a followup because it shouldn't impact the overall solution for "CM should not be creating a new revision when the entity is in (some sort of) a syncing process".
Comment #67
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks for poking this again @amateescu. I think it makes sense to flip the dependencies and block the docs behind the other two issues, since they are used in examples in the latest patch: #3057483: [PP-2] Better describe how SynchronizableInterface should be used for content entities.
Comment #68
plachIt seems to me that the proposed solution does not fully address the issue originally reported by @dawehner: Don't force new revisions automatically in content_moderation. The current solution provides a way to skip the "offending" (no offense intended :) behavior only when an entity synchronization is happening, which makes sense but may not be the only case where avoiding the creation of new revisions might be needed, since synchronization is shaping up to specifically indicate an entity save not happening in the usual user-driven update context.
If we consider our famous example of an hypothetical autosave module relying on revisions, a possibility to save storage space could be to only create a new revision on the first autosave, while all the following ones could update the autosaved revision. This would be problematic if CM were enabled and I don't think it would be reasonable to mark the entity as syncing in this case, to avoid issues with CM, since this could have all sorts of unwanted side-effects.
All this to say that I'm concerned that the current patch and the accompanying CR seem to be promoting
::setSyncing()
as the official way to skip new revisions in CM, regardless of context (see #50 for an example), which I don't think is correct. IMO the correct way would be to ensure that::setNewRevision(FALSE)
works, if called after CM. I agree with @amateescu when he says that we need to ensure that CM works correctly even in programmatic contexts (e.g. API first), so I'm not advocating changing the current behavior to respect a previousFALSE
value, however a test proving that invoking::setNewRevision(FALSE)
after CM applied its logic indeed prevents a new revision from being created would be a good way to make the fix more complete. In this light the CR should be updated accordingly to promote::setNewRevision(FALSE)
as the "official" way and mention syncing as an alternative to be adopted in very specific conditions.Thoughts?
As a side note, I agree with @catch in #31 that I would not expect
node_mass_update()
to update existing revisions, because for auditing purposes it would be better to keep (even limited) history when dealing with a user account cancellation. I think it makes sense to address that in its own issue especially if, as it seems, there are data integrity problems involved. This is why I suggested that #3062900: The combination of content moderation, translations and cancelling user accounts is broken. might actually be critical after all.Comment #69
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented@amateescu and I discussed this in quite a bit of detail. I'll try summarise some of the points made during that discussion:
SynchronizableInterface
, that is updating some historical revision with content pulled from some external system associated with a particular revision.pre_save
hook for these reasons:Overall, I think there may be scope to explore this further outside of a syncing entity, but I think it would be best to explore in the context of a really concrete problem where we can: a) completely eliminate syncing as an option and b) interrogate the use case enough to rule out creating a new revision as the actual preferred option (as has been discussed with
node_mass_update
).As a side note, CM being somewhat opinionated and overriding certain standard behaviors was part of the design, see from 3 years ago: #2839371: Programatically creating a published entity doesn't result in a published entity.. I know @alexpott and I also discussed and came to terms with this in other contexts as well.
Comment #70
maximpodorov CreditAttribution: maximpodorov commentedBTW, is migration a syncing process?
Comment #71
plachThanks for the summary @Sam152 :)
tldr; below more details follow:
::setNewRevision(FALSE)
solution as the "official" one in the CR (yet). However, I think we need a closing paragraph honestly stating the current status: i.e. syncing is a new API and you need to carefully evaluate whether using it, especially because it might have unexpected side effects, as core/contrib adopt it over time. The CR should also mention the alternative solution "in case syncing fails or does not apply" together with the drawbacks outlined in #69.Most points expressed in #69 make sense to me but overall they do not fully address the concerns I expressed in #68, since they are basically saying "we don't know whether there is a valid use case for skipping new revisions besides syncing". Meanwhile hypothetical devs with this use case might abuse syncing to get their logic working. If I were to write custom code (or contrib explicitly supporting CM), I would be fine with hardcoding a few assumptions about the system internals to make the logic work, even if that requires messing with hook weights. This is not an ideal solution but it's supported and it's been relied upon since D7 (or even earlier in some more primitive form).
That's why I talked about the "hypothetical" AS module: it's just an idea of how a revision-based AS module could work. The same module we used while working on the revision tree design. In that context, the AS module does work (at least theoretically) with CM and WS, and I think the use case I described is valid because the revision tree implementation, as we envision it so far, would not address that specific problem.
+1
True but we might also have a counter-example:
node_mass_update()
does seem to fit our current definition of syncing operation, however we would still need new revisions to be created (not sure about updating thechanged
timestamp). Luckily we would have to explicitly set the new revision flag to TRUE anyway, since we cannot rely on CM being enabled.Makes sense to me, but I think we need some kind of "temporary" solution until we flesh this out.
Comment #72
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI'm happy to write extra content for the CR that clarifies the situation and developmental status of the whole syncing paradigm.
I still feel weird about writing and supporting a test that codifies implementation details of CM, I feel like it sends the wrong signals to implementors. For modules that reach into other modules implementations or swap low level components out, IMO they really need to be testing, understanding and supporting the impact of those low level changes as well as being prepared to chase HEAD on those changes if they are ever evolved. Adding that test to core kind of limits the ability to evolve the implementation, since I think a committer would rightly be concerned about removing or altering a test case to coincide with a change in what should be perceived as an implementation change only.
To address this, my preferences in order would be:
Sorry to be a stick in the mud, but if you still feel strongly about the test after reading these points I'll defer to your judgement.
Comment #73
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedI think another way of expressing my general concerns is: developers would be totally within their rights to stonewall changes in content moderations architecture in the future, based around their own assumptions and low level changes to the module, if we advertised replacing content moderation hooks in CRs/tests. I don't know what the scope of those assumptions might be, they might be surprising or incompatible with future features or architectural changes (just like cores treatment of pending revisions had to be reconsidered based on how AS chose to use them).
One completely anecdotal but real example is, I support a site that simply removes content moderations implementation of
hook_entity_access
. It's completely up to me to test and understand the impact of that and it's not something I expect to be tested, supported and catered towards in core.Comment #74
plach@Sam152:
I don't feel as strongly as you do about this issue, but would you consider at least a "manual" test in your local env that messing with weights is actually a valid interim solution to use at own risk until a proper one is figured out? This would be enough to alleviate my concerns and I would be happy to move on with your preferred solution at that point:
Comment #75
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedSure, happy to do some manual testing, attached is the code I used to test this. I've also opened #3070517: Implement a solution for not forcing a new revision of non-syncing content in content moderation .
Comment #76
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedAdded the following to the change record:
I think that addresses all the concerns raised, thanks again for following this up @plach :)
Comment #77
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThank you both for sticking with this :) I think we're ready to go back to RTBC now.
Comment #78
plachSaving credits
Comment #79
plachCommitted 961c2b1 and pushed to 8.8.x. Thanks!
Comment #81
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThanks @plach, publishing change record.
Comment #82
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSince this is a major bug fix which allows Content Moderation to be used together with Workspaces, I think we should backport it to 8.7.x as well.
Comment #83
plachSpoke with @xjm: we both feel that this change is potentially too disruptive to be backported to a patch release. People needing to use both CM and WS on 8.7.x may try to apply the committed patch (and deal with their
hook_requirements()
implementations).Comment #85
xjmSince
SynchronizableEntityTrait::isSyncing
already existed as an entity member, isn't it possible that some modules could already have been doing their own manipulation with this flag, and therefore this issue could change the behavior for them?If so, this issue should probably have the CR updated to add that, as well as a release note added. (If I'm mistaken about this potential edgecase issue, the issue can be untagged and re-closed.)
Thanks!
Comment #86
pameeela CreditAttribution: pameeela commentedAdded a release note snippet with help from @Sam152.
Comment #87
plachLooks good, thanks!