Problem/Motivation
When saving a new revision of an entity to have a moderation state that affects the default state it is possible to inadvertently change other translations of the entity to become the default revision. An update made prior to Content Moderation being committed prevented translations inadvertently becoming unpublished due to this issue, but in turn caused draft revisions to become "missing", although they exist in the database they cannot be accessed via the UI to create a new revision from.
Steps to reproduce
- Enable Content Translation and Content Moderation modules
- Add french language on languages page (i.e. /admin/config/regional/language)
- Enable translation for article node type on content translation page (i.e. /admin/config/regional/content-language)
- Enable "editorial workflow" for article node type on workflow's edit page(i.e. /admin/config/workflow/workflows/manage/editorial)
- Create an article in english and publish ('Save and Publish' button)
- Translate to french and publish ('Save and Publish' button)
- Go to french version of the article. Make changes and save as draft ('save and Create New Draft (this translation)' button)
- Go to english version of the article, you won't be able to add a new revision.
Actual:
Article reverted back to last published version and no draft version available in revision tab (ref: screenshot).
Proposed resolution
Prevent users from changing the moderation state to something that would cause cause a draft revision to become "missing".
Remaining tasks
- Review.
- Improve help text.
- Follow up to rework translations / revisions storage to allow complex workflows.
User interface changes
- Removal of save buttons which will cause issues.
- Additional help / error text.
API changes
none
Data model changes
none
Comment | File | Size | Author |
---|---|---|---|
#202 | interdiff.txt | 2.18 KB | plach |
#202 | 2766957-202.patch | 32.63 KB | plach |
#197 | 2766957-197.patch | 32.39 KB | timmillwood |
#197 | interdiff-2766957-197.txt | 2.98 KB | timmillwood |
#185 | Test_1___Drupal_8_x 2.png | 123.36 KB | plach |
Comments
Comment #2
hchonovPlease take a look at
NodeRevisionRevertTranslationForm::prepareRevertedRevision
.What happens here is the following : the values are taken from the desired revision and copied over, this happens by default only for translatable fields, however the user might say that she wants to override also the non translatable fields. The other translations remain untouched and are carried over as they are into the new revision.
Comment #3
timmillwoodHere's a video where I reproduce the issue: https://www.youtube.com/watch?v=TnzV8eVtmLo
Comment #4
Gábor HojtsyNote that the problem in the video is (as explained by @timmillwood in IRC):
Comment #5
timmillwoodI've been trying to work on a test to prove this, but struggling.
In the attached patch I am adding an english entity, then adding two german revisions where only the first one is set as a default revision, then when adding a second english revision i'd expect the second german revision to become the default, but it doesn't.
This has got me thinking:
- Am I doing something wrong?
- Does the issue only occur via the UI?
- Does the issue only occur for nodes?
Comment #6
alexpottThis has been spotted in the workbench_moderation queue.
Comment #7
hchonov@timmillwood :
You create a new revision and declare it as not default one. Then you load the entity again and you set new default revision -> here you skip the previous not default revision and you are loading the entity in the previous default revision and from it creating a new one default revision. So it behaves exactly as expected and this is why the second german revision can not become the default one, as you've loaded the first german revision and from it created a third revision, so the second one is being skipped and the third default one is based on the last one default revision.
Comment #8
timmillwoodBut in my video #3 it behaves differently.
Currently looking into if it's content moderation module causing the issue.
Comment #9
timmillwoodThis is a core patch for core with content_moderation (https://github.com/timmillwood/content_moderation) installed in core/modules.
I *think* it shows the issue.
Comment #10
timmillwoodRecorded a new video on this https://youtu.be/EI_shX3sQ28
I've noticed that when you add a translation it stems from the default revision, but when you edit it stems from the latest revision.
I'm thinking this is a ContentEntityForm issue, maybe?
Comment #11
timmillwoodPretty sure the issue is in
\Drupal\content_moderation\ParamConverter\EntityRevisionConverter::convert
where the latest revision is loaded, when sometimes we want the default revision.It's all content_moderation's fault after all.
Comment #12
timmillwoodI think if
$latest_revision->isRevisionTranslationAffected()
we should return the latest revision, otherwise the default revision?Comment #13
timmillwoodHere's what I'm thinking:
Going to commit to https://github.com/timmillwood/content_moderation
Comment #14
catchTrying a re-title to describe the bug here.
Comment #15
artem_sylchukHi!
Just want to note that using approach mentioned in #13 you'll probably loose the french draft after publishing english version.
Comment #16
timmillwood@james_kerrigan it's still there, but ends up becoming a past revision, instead of a forward revision, which is a bit strange.
Comment #17
artem_sylchuk@timmillwood I've been playing around this for a some and here what I've found:
When you load the default revision in your param_converter service you actually say to use that revision for all languages.
Look at the SqlContentEntityStorage::saveToSharedTables. During the save process it iterates over all translations of the entity and writes them into the shared table (node_field_revision, for example). So all the records for all languages will point to the revision you are saving.
In your case you save the default revision, node_field_revision table is populated with the rows taken for the translations of the default revision and forwarded ones become outdated.
I've managed to solve this using complicated workaround over the entity storage class, but I don't like that solution. In general, I do the following:
- if we are saving entity in the default moderation state and translation is in not-default state, then I look for the last default-state revision for that language and use it for the record.
- then I save one more revision where I do things vice versa: save non-default-state revisions as they are and load last non-default-state revision for the default ones.
Not sure if this information is helpful, but I think this is not just content_moderation issue, but probably also something about entity storage or ContentEntityBase::getTranslation or ContentEntityBase::initializeTranslation
Comment #18
artem_sylchukSorry for sharing probably irrelevant information, but after thinking a bit more about this I think everything can be kept on the EntityRevisionConverter level.
There is a table 'workbench_revision_tracker' which tracks the latest revisions of nodes. It tracks the revision we are interested in. So we can do something like this:
In EntityRevisionConverter:
And in the RevisionTracker class:
I can provide a patch if this makes any sense.
Comment #19
timmillwood@james_kerrigan my latest code for this is in https://github.com/timmillwood/content_moderation/blob/master/src/ParamC...
If you have a better solution I'm happy to take PRs.
Comment #20
Gábor HojtsyHm, if we need to load different revisions based on language, how does that resolve it?
Comment #21
timmillwood@gabor I don't know how to explain in text form, this whole issue has damaged my brain.
Comment #22
Gábor Hojtsy@timmillwood: can you explain it in spoken words? Maybe talk tomorrow then?
Comment #23
timmillwood@Gábor Hojtsy - Yes, but can't do tomorrow. Monday?
This issue is that content_moderation (and wbm) enforces using the latest revision when loading the edit form. However we really need the latest revision with edits in the current language. So if the latest revision is loaded unless the latest revision of the current language was not edited, otherwise it will load the default revision. This means when editing english you may get revision 3, but editing french you may get revision 4.
Comment #24
Gábor HojtsyBut loading for editing is not the problem is it? The problem is what gets saved back then, which is what James says in #15.
Comment #25
timmillwoodThe revision that gets saved is inherited from the revision that's loaded. The Drupal standard is the load the default revision, but content moderation often needs to load the latest revision to do forward revisions. Although setting the latest revision as default may not be what's wanted across all languages. So sometimes inheriting from the default revision makes sense.
Comment #26
artem_sylchuk@Gábor Hojtsy, sorry for the confusion, please ignore my comment. Revision isn't lost, it just becomes outdated.
My idea was to look for the revision in the revision_tracker table - it stores the latest revision id for every language independently. I've created a pull request to demonstrate what I mean: https://github.com/timmillwood/content_moderation/pull/3 but I had no time to properly test this. I'm going to work on it on the weekend but I'm not sure if this makes any sense at all.
Comment #27
Gábor HojtsyRight, looks like there is no good way to load a revision then if we load the same revision for each language because some languages it may be a forward revision while others it may not be. So merely loading a different revision globally would not solve the problem then as per #25? If either of you can visualize this with a little napkin drawing some revisions and what gets saved and how it modifies the things and why its not good, it would be fabulous for understanding IMHO. The issue summary is very short on details and its hard for me to pick up them chip by chip.
Comment #28
timmillwood@james_kerrigan - Thanks for the PR, I think your idea might actually work. Going to merge it in, update it, and play around with it today.
Comment #29
catchComment #30
timmillwoodComment #31
timmillwoodMerged @james_kerrigan's PR #26 and now trying to make it "work", although found one issue.
Take this example:
1) Create an unpublished english revision
2) Add a published french revision
3) Add an unpublished (draft) french (forward) revision
4) Add a published english revision.
In step 4 of this example we need the english revision from step 1 to be published and made the default revision for the entity, but we still need the french version from step 2. The revision tracker table would actually load the revision created in step 1, without the french translation. In this case we need the revision from step 2, which happens to be the default revision.
Trying to think of an example where we might not want the default revision nor the latest revision.
Comment #32
BerdirI'm not sure such scenarios are possible at all? I thought we discussed that? :)
The revision history is a linear path of updates and it doesn't care about languages, meaning, that every revision has all the languages that the previous revision hat, plus whatever has been updated in this revision. You could create a new revision based an old revision, but then everything inbetween is lost.
So for step 4, you can decide between starting on revision 1, then you have no french at all, revision 2, then you have the published french translation or 3, then you have the new french version but then that's also what will be published. If you chose 2 for example, then the changes in revision 3 are *gone*, and as far as the french goes, 3 would be a revert of 2. To get them back, you'd need a *new* forward revision that would take the french changes between revision 2 and 3 and re-apply them on top of 4 and save as revision 5. You might even be able to do that by using the UI to revert revision 4 to revision 2 only for the french changes.
Or put differently, step 3 is an illusion. What you did there is a forward revision of the french translation *and* the (unchanged) english original. You can not load an entity in a language, you can only load a revision and with that revision, you get every translation it has. And when saving, you save back every language.
That's why I don't like the changes that were done with the language filtering in the revision list. It suggests functionality and concepts that simply do not exist.
Comment #33
Gábor HojtsyRe #31, I am looking at your example. Not sure how published / forward revisions are supposed to work across language. What would users experience in step 2 when English did not even yet have a published revision? So long as you have a pointer for published revisions per language, why is it a problem that the latest published revisions are on different revision numbers per language? I am not sure you can get around that unless you always save a new revision just for the sake of having them all at once like D7 did fake forward revisions.
Comment #34
Gábor HojtsyNot sure if the process changed since then, but looking at #218755: Support revisions in different states which apparently I helped develop :D default revision of an entity is not per language. So indeed, for languages changes to happen like that, the module dealing with default revision would need to copy the right data from the right revisions per language (that is, once again, this is a saving problem not a loading problem).
Then whether content moderation, etc. let you deal with the "prior" translation variant as a "forward" revision although technically it is an older revision (due to the default revision needing to copy the published content) is a different matter. If content moderation only works with revisions posted since the default revision then translations in different publish states / forward revisions is impossible, because they would always need to be published (made default revision) or have forward revisions at the same time.
Comment #35
Gábor HojtsyI created a figure about this to help visualize. Am I getting it all right? :)
Comment #36
Gábor Hojtsy@timmillwood: so that said,
Comment #37
catchSo I think Berdir's right that we should not get into a situation where:
1. Published English node
2. Draft English revision based off #1
3. Draft French revision based off #2
4. Publish Engish revision
5. Publish French revision (overwriting #4 with #1)
This requires forking the content into two branches (#2 and #3) and we can choose to not support that in the UI. i.e. you either make changes to all translations and publish them, or you edit and publish translations sequentially, but you don't get to edit and publish translations both simultaneously and independently. Contrib or later core functionality could open this up again, but it feels like a problem we can not have.
This is really no different (once you get to workspaces) to:
1. Published English node
2. Draft English revision based off #1
3. Separate draft English revision based off #2
4. Publish #2
5. Publish #3 (wiping out changes from #2)
In that scenario we have no way to know if #5 was intended or if the changes were somehow supposed to be merged - again, can choose not to have that problem.
Comment #38
Gábor HojtsyWhat is the user interaction then on editing and/or deployment?
1. What happens if an entity already has a draft French and now want to publish a new English but keep the draft French? (Scenario from #31/#35)? Would the UI say, no you cannot publish English because either you publish all of them or none of them?
2. What happens if the live site has a French draft (not English) and the staging site has a new published English (no change for French) and then you deploy content from staging to live? So basically the same as 1 but not in the entity editing process but through content staging? The two environments cannot independently say what happens on either of them is invalid, because it is totally valid. But the merge would not be valid according to what you propose(?)
Comment #39
catch#1. If you have a draft French translation (that's never been published), then that should be able to persist through edits to the English translation. The revision history is still linear in that case.
If you already have a draft revision for the French translation, then yes you would not be able to create a new draft revision for the English translation without taking the French along with you. Not that we stop contrib or later core patches from trying to handle that, but it's a different use-case to one-language-at-a-time or all-languages-at-once.
#2. You can get into that situation with English too (an edit on the live site, and an edit on the staging site). If you're using content staging with a separate staging site, but also making edits on the live site to the same content you're staging, then... why would you do that? Can't stop people but don't see any reason to support this case.
Comment #40
Gábor HojtsyUpdated figure wording as "draft revision" instead of "forward revision" based on feedback from @catch:
Comment #41
catchWe need to drop the concept of forward revisions I think, and move to:
Live
Pending (not live yet)
Archived (was live previously)
Abandoned/Discarded (never going to be live)
The problem with the 'alternative future' in #40 is you now have that pending French revision left hanging.
If you edit the node again, if it's based off the hanging French pending revision, it'll have the old English content. If it's based off the live revision, it won't have the new French content.
#37/#39 is trying to get at that but didn't manage to express it clearly.
What I think we need to do as a first pass:
If there is a pending revision (regardless of language), and you edit the node:
- inform the user that they're editing based on a pending revision with changes in x languages, and if they want to edit based on the live revision, present the option to delete (or put in a discarded state if/when we add that) the pending revision
This should be the case regardless of the language the changes are made, and it ensures there's always a linear revision history then.
This means as Gabor points out, if there's a pending revision with changes in French, you have a choice of not editing the English until the French revision is published (or discarded/deleted), or bring the changes along together. It seems valid to have draft changes in both French and English, which then get published together. What you can't do is publish English or French revisions leaving pending revisions in either language, then except to be able to publish those pending revisions successfully.
In irc Tim Millwood suggested a UI where you can choose the revisions your draft is created from. So for example if we added that 'discarded' state. You could create a new draft based on published English and the discarded French pending revision - which allows for that history rewrite. That's a complex interface to present to people, but would allow for a non-linear editing workflow, while still keeping the actual revision history linear.
Comment #42
Gábor HojtsyI think this is fine as a first step. It would be unfortunate if this would be the way it works forever (one would need to coordinate work always between their translators and original doc changes), but for now IMHO its fine to move forward.
Comment #43
timmillwoodI think this is better (for initial commit) than promoting a pending revision to live.
Which is why the 'alternative future' suggestion in #40 is what the patch in #2725533: Add experimental content_moderation module does.
I think @catch's "first pass" suggestion should actually be the second pass.
Comment #44
alexpottDiscussed with @xjm, @catch, @effulgentsia and @cottser. We decided this should not block #2725533: Add experimental content_moderation module from being an experimental module in core. We think that in order to do this properly and not rushed we should just add a warning to the content_moderation module if the site is multi-lingual.
Comment #45
Gábor HojtsyWell, there is already a short term fix in #2725533: Add experimental content_moderation module to avoid loosing your default revision, so it only looses the fact that some now older revisions are future drafts. While that is impossible to recover from, it is much lesser of a data loss than before :)
Comment #46
timmillwoodIn #2725463: WI: Add StatusItem field type to store archived state I am looking to change the "status" field to store if a revision is a "forward revision" / future draft / pending, whatever you wanna call it, as well as storing if a revision is archived (which is the original purpose of the issue). The change will help this issue as we will be able to tell which revisions have been the "default" and which haven't.
Comment #47
chanchal2002 CreditAttribution: chanchal2002 commentedGetting this issue in 8.1.x
Comment #48
alexpottComment #49
alexpott@chanchal2002 I guess you have workbench_moderation installed? Or do you have another way of causing this?
Comment #51
timmillwoodComment #53
dawehnerIn order to avoid this problem I wrote a little module (https://github.com/dawehner/content_translation_worfklow). When you publish from a draft revision, it copies over all data for all other translations from the previous published version. This causes "just" some issues with the UI, but at least doesn't unpublish other languages.
Comment #54
dawehnerOn top of that there is also a module which allows to actually see what is going on between revisions and translations: https://github.com/dawehner/content_translation_revision
It shows for each revision each translation and its moderation state. Once you have just this module enabled, you can see the problem of this issue quite clearly.
Comment #55
alexpottDiscussed with @catch and @xjm. We agreed that this is actually a critical issue since unexpected unpublishing of content is hard to detect and could break sites.
Comment #56
timmillwoodThe issue described in the video posted in #3 isn't an issue anymore.
Updated the issue summary to try and reflect the current state.
I think the way forward with this might be revision parents (#2725523: Add a revision_parent field to revisionable entities) and a revision graph, then we'll know exactly where a revision came from, but it's still a complex issue.
Comment #57
timmillwoodIf we were to have revision parents and revision graphs with the current implementation of Content moderation we'd have something like this.
This shows that revision parents and revision graphs will not solve the issue, but might help manage the data.
Comment #58
catchI'm not sure 'forward revisions becoming past revisions' is the right way to frame this - the issue is that the translation UI allows for revisions to be forked.
Once we have a draft revision, we should enforce that edits and translations are made against that draft revision, not back against the published revision again. If we do that, we don't get into the situation of trying to deal with merges. That way we continue to have a linear revision history without orphaned changes.
There may be sites that need to 'hotfix' content while keeping pending drafts with larger changes, but that's something that can be done in contrib (or in a later minor release, since it's a feature, not necessary to fix this critical bug that results in data loss).
Comment #59
timmillwood@catch - took me 3 reads of your comment, but if we're both thinking in the same way that might just work.
/me goes to write a test.
Comment #60
timmillwoodAs part of this it might be worth trying to fix #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types, because what is the latest revision, and also, maybe more importantly, what is a draft revision?
Should we add a
draft_revision
field to\Drupal\Core\Entity\RevisionLogEntityTrait
, which is 1 if it has never been the default revision, and 0 once it has been a default revision?Comment #61
timmillwoodNot sure #58 will work, trying to prove it.
Comment #62
timmillwoodIf we do this we will end up with the same issue described in #3 which was resolved prior to Content Moderation's initial commit.
Comment #63
timmillwoodLet's take this example, revisions 1-5, 0==unpublished, 1==published, X==no revision created yet.
Therefore the solution I'm suggesting is enforcing only one language to have forward revisions at any given time.
Comment #64
dawehnerI'm sorry, but this doesn't work on any complex workflow. This basically makes workflow monolingual.
Comment #65
catch@dawehner we can try to support complex workflows later, but I think we should fix the critical data-loss first in the simplest way possible. Note also this general issue only affects translations once they've already been published - you can have multiple unpublished translations in a draft without running into problems.
Comment #66
timmillwoodThe restriction of only one language having forward revisions would be done at a form level. Specifically in
\Drupal\content_moderation\StateTransitionValidation::getValidTransitions
where we'll prevent transitions which go to a non-default revision if forward revisions exist.This should be pretty easy to switch out in contrib (which I might even do as part of this).
Comment #67
timmillwoodInitial patch based on the proposal from #63.
It prevents a transition to a state which creates a forward revision if the entity being edited is a default revision and has forward revision.
Now to write a test for it.
Comment #68
timmillwoodComment #69
timmillwoodBecause the patch in #63 is form related it'd be great to get #2753717: Add select field to choose moderation state on entity forms committed first, anyone wanna review?
Comment #70
catchSo theoretically at least this should allow for multiple translators to work together, i.e.:
What it doesn't allow is this:
@dawehner does that help clarify the exact limitation in regards to #64?
Comment #71
dawehner@catch
Ah I see. That is indeed better than I initially though. As a result though we have translations being temporarily not accessible on the website. That itself for me is a critical follow up, don't you think so?
Comment #72
timmillwoodWhile writing the test I've found #67 doesn't fully work as expected. Don't think my head has the space to explain just yet, just wanted to make it noted.
Comment #74
catch@dawehner not sure what you mean. Any translation can be worked on in draft format and published, they're temporarily not available because they're drafts. Something you can't do is this:
You'd either have to publish the changes to Korean and French at the same time, or work on the edits and publish them sequentially. Is this what you meant or something else?
Comment #75
timmillwoodI can't think of a good solution, apart from preventing any entity saves. Maybe if I explain it'll help.
If we now go to create a new english revision the patch in #67 gives us one option "Save and Publish". If we did this revision 4 would get created with english published and copying the french revision 2. Therefore we're back to the current problem in HEAD, 3 (the forward revision) would become a past revision. If the option on the english node from was "Save and Create new draft" we'd have a similar issue, revision 4 would have an unpublished english, but copy the published revision 2 from french. Revision 2 would still remain the default but the french changes in revision 3 would be "lost".
I think the only solution would be allow no editing of the default revision if there are forward revisions.
Comment #76
catchYes, we should prevent this IMO. Contrib can open it up and/or we could add a confirm form or something
Question for me on #75 is is - why can we not (at least have an option to) create revision 4 (the new English revision) based on revision 3, instead of revision 2? Then the changes in both languages will be preserved.
Comment #77
timmillwoodI guess we could create revision for based on 3 instead of 2. We'll have to work out the logic for if or when that should be the case.
Comment #78
dawehnerThis is the particular workflow/bug I have in mind.
Please ask, if this is not clear.
Comment #79
catchNo that's clear, to keep linear history, you could do this though:
Comment #80
timmillwoodHere's a progress patch, it actually breaks more than it fixes.
What I am trying to do is when there are forward revisions only allow transitions that keep the default revision status the same. So if it was the default it will be again, if it wasn't it won't be.
I've also update based on #76 to copy the latest revision for translations rather than the default revision.
Feel free to play.
Comment #82
timmillwoodAnother progress patch.
Comment #84
Gábor HojtsyYay, great to see this moving! We'll definitely need some kind of UI affordance to tell people why they cannot do certain things or they will freak out and submit bugs. Also:
I was quite confused pairing up the parenthesis here for a bit. Can you indent this according to the logical grouping happening?
Comment #85
timmillwoodWhat sort of UI affordance are you thinking?
drupal_set_message('You cannot publish this content because there is a draft revision in another language which needs publishing first');
I don't think this is ready for usability review but I'll join the next UX call to gather some ideas on how we can make it clear WTF is going on.
The StateTransitionValidation code is really confusing, I'm working to try and clear it up.
Comment #86
Gábor HojtsyWell, if we would already be dealing with #2753717: Add select field to choose moderation state on entity forms then I would say adjust the description of the text field to explain what is going on. We are not there yet unfortunately with #2753717: Add select field to choose moderation state on entity forms. Setting a message at the top of the page may be too far from the button for people to make the connection IMHO.
Comment #87
timmillwoodYay, tests pass, but still not 100% confident in my logic.
Comment #88
timmillwoodAs you can see from the test in #87, this covered a lot of use cases, but one is doesn't cover is when all languages are published.
In the case above 1 is the default revision with both english and french published. We can create a forward revision of english (revision 2), then go to the edit form for french and I don't think there should be any valid transitions. Therefore should it just not show the edit form at all?
Comment #89
timmillwoodSo this patch solves the issue in #88 (with tests), but it does also mean we can get an entity form with no save button at all. As I asked in #88, should it just not show the edit form at all?
Comment #90
timmillwoodWe can get into the same situation of a user having no save button if a user has permissions to create / edit content, but not one of the following content moderation permissions:
Comment #91
hass CreditAttribution: hass commentedReally? I need to call someone by phone and please him to publish his content in spanish before I can proceed and publish my content in German? He may needs approval by someone else first and I need to wait some weeks before I can fix serious content bugs? That does not fit in any translation workflow. Not everything is a one by one translation and not every translation need to wait for other translations as they are totally independed from each other. Several translations may be done by totally different people with different skils.
Comment #92
timmillwood@hass - I'm open to better solutions, and this restriction only comes into force if a data loss would occur from the save.
Comment #93
catchDiscussed with @alexpott, @cilefen, @cottser, @laurii, and @xjm and we agreed on the critical status due to the data loss/data integrity.
@hass we can open another issue to try to support advanced workflows, but we need to fix the critical and unexpected data loss with relatively simple workflows first.
Comment #94
AMDandy CreditAttribution: AMDandy commentedWhen you have multiple revisions in draft state are you still able to publish them all out? The current resolution without this patch is to quickly publish each forward revision before anybody notices that there are unpublished revisions out there but that would seem to break if the user is not allowed to transition content to a published state.
Comment #95
timmillwood@AMDandy - This patch prevents you from having multiple revisions in draft state.
Comment #96
AMDandy CreditAttribution: AMDandy commented@timmillwood the cure is almost worse than the disease, isn't it? Currently users with knowledge of the bug can still translate a new revision in all languages and quickly publish them out triggering the undesired behavior but only momentarily. With this patch applied a new product launch that requires updating a site's homepage could not be staged in all languages ahead of launch, right?
Comment #97
timmillwoodAdding "some kind of UI affordance to tell people why they cannot do certain things".
Comment #98
dawehnerMaybe suggesting people to publish the content or revert the draft would be helpful?
Comment #99
dawehner-
Comment #100
dawehner-
Comment #102
Bojhan CreditAttribution: Bojhan commentedYhea, I prefer to always add a "How to resolve this" part to a error. Not just claim whats wrong.
Comment #103
hass CreditAttribution: hass commentedI'm shocked. Multilingual sites are not translated this way. We start with English and do translations in every thinkable direction like Spanisch, French, Italien, German in parallel with several translators. But a German translator may not able to translate to any other language than German. He cannot wait for other translators and other language translators not for him. We may be also in situations where we have law related text on a website that cannot wait to be edited just because a translator of a different language started with translation and has not finished his work yet.
This is not an advanced workflow process. This can only be a serious design flaw! This need to be fixed.
Every language need to be totally independent from each others. I'm not sure I'm expierenced enough with content moderation module code to share a patch to solve this bug, but this is a show stopper for everyone that really makes no logic sense. This need to be an absolute blocker for the content moderation module.
Comment #104
dawehner@hass
Please calm down. Yes this issue doesn't solve the usecase for example I had with the two multilingual sites with moderation enabled so far, BUT it solves a concrete problem. Please open up a follow up which describes your usecase and be constructive. If this would be an easy problem, we would just solve it here, but apparently its not.
I think this issue desperately needs an issue summary.
Comment #105
timmillwoodThis issue is on my todo list for today:
- update issue summary.
- improve wording of help text.
- open follow up.
- investigate if we can have a default revision per language in a BC way with upgrade path.
Comment #106
timmillwoodComment #107
timmillwoodFollow up created #2860097: Ensure that content translations can be moderated independently.
Comment #108
timmillwoodUpdated the wording, also removed unrelated changes to
ModerationInformation
.Edit: embeded the wrong image.
Comment #110
timmillwoodOoops, when updating help text it'd be good to also update the test.
However before I go rolling another patch it's be good to get sign off on the final text, so switching back to "needs review" for that.
Comment #111
hass CreditAttribution: hass commentedThe placeholder @invalid and the sentence is very difficult to understand without context on d.o. This makes translation very difficult. Better name it @transition_label or @transition_name or so. Or rewrite the sentence.
Comment #112
timmillwoodGood suggestion @hass, thanks, I'll add that in the next patch.
Comment #113
dixon_Comment #114
timmillwoodThis latest patch improves the error message with links to where to resolve it.
I have not yet addressed #110 or #111, so this patch will fail testing, but my kids want their dinner.
Comment #116
hass CreditAttribution: hass commentedI think this not allowed. Never change casing of strings. Transitions are translatable strings and/or may have casing that must not altered. If you'd like to use it inside a translatable string and feal it need to be lowercase inside an english string, use %placeholder, but leave the label as is. Please do not make it lowercase only because the *english* sentence require it to be lowercase. :-)
Comment #117
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #118
hass CreditAttribution: hass commentedUse %placeholder or someone may add the lowercasing back... :-(
Comment #120
timmillwoodFixing #110 #111 #116 #118. Also removed all form elements if there is no valid transitions.
Comment #121
yoroy CreditAttribution: yoroy as a volunteer and at Roy Scholten commentedThis is head-hurtingly complex :) I was going to ask about can we fix this without having to warn or stop users doing their thing but I that around #93 to #102 it is acknowledged that would be nice but this is not that issue.
So on to the actual user interface changes:
- Thanks for adding the links to the message, very helpful. (but: I did not recreate a situation where this actually happens, so I didn't get to actually click those links)
- "create new draft (…) this content" is weird and on first read I didn't understand "create new draft" as being one of the moderation states. Maybe '%invalid_transition_labels are not possible for this @entity_type_label.' is better? That would end up as "Create new draft, publish or archive are not possible for this content."
- Content is shown with a capital C, should probably lowercase that.
Comment #122
timmillwoodYes, it is head-hurtingly complex
- To recreate this have two languages both with published revisions, then create a draft revision on one of the languages, on the other language try to create a draft revision and you will hit this.
- Yes, it does sounds weird, these are the transition labels, the only other place "create new draft" is used is on the "Save and Create New Draft" button. I wonder if we change the transition label to "Create a new draft" it might sound better? as in "It is not possible to create a new draft, archive, or publish this content" and "save and create a new draft".
- "Content" with a capital C comes from the Node entity type label. Can someone advise me on if it is ok to just strtolower() that?
Comment #123
timmillwoodPostponing on #2862988: EntityOperations::entityPresave doesn't always set the correct default revision. With revisions getting set default when they shouldn't be we have an unstable platform for testing and fixing this issue.
Comment #124
timmillwoodAdding tests for this based on the bugs exposed by #2862988: EntityOperations::entityPresave doesn't always set the correct default revision.
They're not passing locally, so need to do some more work to make sure the logic is right.
Comment #125
timmillwoodComment #126
timmillwoodComment #129
timmillwoodThe failing test in #126 is really baffling me. I get the same fail locally, but if I manually test I don't get the issue.
Running this through xdebug I can see the offending revision is returning TRUE for
$entity->isRevisionTranslationAffected();
when it shouldn't, this is causing all transitions to be available rather than just "Create New Draft".Comment #130
timmillwoodIt seems to be in the test we update the body each time we create a new revision, therefore both languages become revision_translation_affected. Therefore
($this->moderationInfo->isLatestRevision($entity) && $entity->isRevisionTranslationAffected())
returns true for all languages and all transitions, which is not good. Looking for some alternative logic.Comment #131
timmillwoodI give up!
I know exactly what's happening and why it's happening, but I just don't know the solution. Happy to talk to anyone who wants to take a stab at it.
Comment #132
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis is a patch that requires #2784201: Track the latest_revision and latest_revision_translation_affected ID in the entity data table for revisionable entity types. I believe it fixes the root cause of the data loss by loading the latest translation affected revision via the param converter.
I think the param converter should be moved to core with BC, so this is just a proof of concept at this stage.
Comment #133
timmillwoodPostponed on #2862988: EntityOperations::entityPresave doesn't always set the correct default revision, but now that's in.
Comment #134
timmillwoodComing back to this issue today I think the only way forward, if we want to pursue this as an initial fix is to lock down all translations and only allow drafts on one at a time if one translation has been published, which is crazy, but the only way we can be sure to prevent this issue from happening.
Comment #135
dixon_This issue was discussed during the Hard Problems meeting at DrupalCon in Baltimore. See notes and decisions here: https://docs.google.com/document/d/1-5B-Gndhklns20NZpGQul10525rFWuYB2NbT...
Comment #136
AMDandy CreditAttribution: AMDandy commentedWhy would a restrictive approach like this be preferable to something like the content_translation_workflow module's approach? The capability to stage multiple translations at the same time is crucial to our site and this approach is a non-starter for us.
Comment #137
timmillwoodImagine a case where there are 10s of forward translations and 10s of languages, then a translation is published and we have to generate 100s of revisions.
Comment #138
AMDandy CreditAttribution: AMDandy commentedSuch a case can be worked around by training authors/editors. A patch this restrictive completely disallows staging edits in multiple languages and has no way around it. This change would change Drupal from being a great solution to being completely nonviable for our site.
Comment #139
timmillwoodTrying to simplify the patch.
Comment #141
timmillwoodForgot to remove a hook.
Comment #143
catchAMDandy you can stage multiple translations with this restriction, you just have to publish them all at the same time, and not make changes to the original language while doing so. It's restrictive, and we should try to do better than this in core, but we should get to a point where there isn't critical data loss and build out from there.
Comment #144
tacituseu CreditAttribution: tacituseu commentedUnrelated failure.
Comment #145
timmillwoodStill looking for reviews and manual testing of this.
Comment #146
timmillwoodIt'd be great to get some screenshots of the output given in #141 when someone does manual testing.
I wonder if it's worth putting the "publish or delete" links in the DSM.
These need addressing too.
Comment #147
vijaycs85- Re-rolled the patch
- Updated issue summary with "steps to reproduce" section
- Tested as per "steps to reproduce" and managed to save both original and translation drafts(ref: screenshot)
Please let me know, if I am missing any as the expected behaviour of the patch is to get error on second forward revision.
Comment #148
vijaycs85@timmillwood pointed out that I didn't have content moderate module enabled and removed block of code as part of re-roll. So updated steps to reproduce and re-tested with updated patch and still able to have two drafts (ref: screenshot).
Comment #151
catchI've opened #2878556: Ensure that changes to untranslatable fields affect only one translation in pending revisions to try to define at a high level what some of the behaviour should be for publishing drafts and reverting published revisions.
Comment #152
vijaycs85Fixing test fails. timmillwood++
Comment #153
timmillwoodHere's a screenshot of the message:
I got this by following the steps to reproduce (which I've updated)
Although I noticed if you do a different order (english published, french published, english draft) then you don't hit the issue.
The patch in #148 mistakenly removes sites/example.settings.local.php
Comment #154
timmillwoodFixing #146 and #153.
Comment #155
vijaycs85Thanks for the update and fixing the settings file mess @timmillwood. I just tested the patch in #154 and looks working as expected.
Attached screenshot for all 4 scenarios.
Comment #156
timmillwoodThank @vijaycs85, glad it's working for you.
Comment #157
catchJust an interim review, the logic looks OK I think, but I feel like we're hitting limitations we don't need to due to the UI looking at the test coverage (see last question).
This could use a summary of the logic inline as well as why we do it.
s/latest revision/draft revision/g?
If there are draft French and English revisions, then really the following should be possible without causing any data-integrity issues:
1. Draft french revision
2. Draft English revision (based on 1).
3. Draft French revision (based on 2).
4. Publish them together.
Comment #158
timmillwood@catch - I assume in #157.3 you're suggesting the 4 steps come after publishing both an english and a french revision? In the current implementation some of these draft revisions will be based on the default revision rather than the latest because of how the param converter is setup. We could change this to always take the latest revision.
Although we don't (as far as I know) have a way in core to update two translations in one transaction. So we would not be able to publish them together. We could add a "Save and Publish (All translations)" button? Not sure how this would go down.
Comment #159
timmillwoodThis patch fixes #158.1 and #158.2, it also fixes some things that go lost in a re-roll.
Comment #161
timmillwoodHelps if I create the patch against 8.4.x.
Interdiff from #159 is still correct.
Comment #162
catchSo yes definitely this I think would remove some of the limitations this patch adds - we're only enforcing a linear version history here.
This is a UX/product/content_translation issue I think. Maybe a new follow-up similar to #2878556: Ensure that changes to untranslatable fields affect only one translation in pending revisions?
Comment #163
timmillwoodThe linear version history is resolved in #161.
I agree "Save and Publish (All translations)" should be a follow up.
Comment #164
vijaycs85Created follow up at #2885480: UI improvement to update multiple translations with content moderation
Comment #165
Bojhan CreditAttribution: Bojhan commentedCan we move from "It is not possible to save this Content." to "Unable to save this Article." ? We should have singular forms of entities?
Comment #167
timmillwoodRe-roll of #161.
Comment #168
timmillwoodUpdated the text as @Bojhan suggested in #165.
Also found a bug when you have multiple translations where each of them has the latest revision with revision_translation_affected set to NULL. This can happen when one translation is just copied over (thus the normal case for a NULL revision_translation_affected) and another translation has been saved with no changes. I have added test coverage for this, so this patch will fail.
Any suggestions for solutions welcome.
This is similar to the issue we're facing in #2881221: Moderating unpublished nodes disregard log messages and do not create new revisions.
Comment #170
timmillwood@hchonov @plach and I had a discussion today about this.
The issue in #168 comes down to not knowing which translation is the draft in the latest revision. So we came to the agreement that this should be the first step.
As @hchonov detailed in an email to me over the weekend, we should introduce a
revision_type
field. This would map to "draft" or "default" with potential for more contrib or core types. We could then use this data to throw the error as described in the patches of this issues.The next step would be to only have one translation per revision. We discussed that (at least initially) this would just be for draft revisions. This would allow translations to progress independently of each other.
Once we split translations per revision we'd then need a way to merge them, as a default revision would still have multiple translations in one entity.
Comment #171
timmillwoodMoving back to needs work to discuss the introduction of a new field (in a new issue) or if there is a bug with the revision_translation_affected field? also for more discussion with @plach and @hchonov.
Comment #172
xjmComment #173
plachYesterday I spoke about the approach outlined in #170 with @catch. In general he was onboard with the approach we discussed. We agreed that:
revision_translation_affected
field. Every translator can create independent sequential drafts to be progressively merged into the default revision, as soon as they are published.{node}
.@catch noted that probably in core we don't need a
revision_type
field, in fact adraft
boolean field would be enough. I wasn't able to come up with use cases for having other states than drafts/default, but I told him that @hchonov may have more to say around this topic. Anyway, this should not be a big deal, as long as we don't need to edit past revision records every time we save a new revision. This should be avoided at all costs, as it may easily end up introducing race conditions.One area that was concerning me was the upgrade path from D7 sites using Drafty/CPS or Workbench Moderation, as at least the former allows to have multiple translations living in the same revision. We agreed that we don't need to support automated migrations in core and that, when writing a custom migration, a possible way forward would be to split D7 revisions into multiple D8 revisions. This should allow to keep the modification history more or less intact.
We also discussed untranslatable fields and #2878556: Ensure that changes to untranslatable fields affect only one translation in pending revisions. Since Content Moderation is all about implementing complex editorial workflows and controlled content management, I suggested that we focus mainly on enabling a proper translation workflow, i.e. via the stripped-down entity forms that the Content Translation module provides to users having translation permissions but not editing permissions. These forms allow to edit only translatable fields, which would allow to "easily" merge the revisions created by the various translators, as no conflict could happen.
We would still need to support occasional changes performed by "editors" (people having edit permissions on the entities), however in these cases we should rely on them knowing the inherent risks of editing fields shared across translations. To alleviate this need we could provide warnings or leverage conflict management when we #2862574: Add ability to track an entity object's dirty fields (and see if it has changed). On top of that we could add an option (already provided by ET in D7) to hide untranslatable fields when a non-default language is being edited.
I hope this summary makes sense. This is a very complex area and it would be great if the people that expressed their concerns previously provided a detailed description of the translation workflows they currently have in place (or they would like to implement in an ideal world), so that we can validate the current approach.
Comment #174
timmillwoodI've opened a new issue #2891215: Add a way to track whether a revision was default when originally created for creating the revision_type / is_draft / draft (or whatever we end up calling it) field.
Comment #175
catchOnly thing I'm not sure about from the summary is this - what I'd expect is we only allow one language to be changed per revision save, but each revision contains the sequential history of changes in other languages - i.e. if there are 'concurrent' French and English drafts, they're actually a linear, cumulative revision history for the entity as a whole, and loading the latest draft revision would contain all the draft changes in both French and English.
Comment #176
plachActually new drafts are copied from the default revision, so the approach in #173 should be correct (attaching a CSV export of my local
node_field_revision
table). Even if it weren't the case, once we know via the r_t_a flag which language the revision affects, we can safely ignore field values in the other languages when merging translation data in the process of creating a new default revision.Comment #177
timmillwoodCore loads the default revision on the entity edit form. Content moderation changes this to load the latest revision if it's revision_translation_affected. The patch here changes that again to always load the latest revision.
Comment #178
timmillwoodIn #2891215: Add a way to track whether a revision was default when originally created we discussed the idea of when all revisions are revision_translation_affected (RTA) NULL going back to find the latest RTA revision. This is not really going to plan, because we only really want to go back to the latest RTA revision for *the* draft revision, not any translation, and we still don't know this.
My next step will be scrapping this idea, and trying to set RTA 1 in
\Drupal\content_moderation\EntityOperations::entityPresave
.Unless anyone has any better ideas.
Comment #179
hchonovFrom #2891215-16: Add a way to track whether a revision was default when originally created:
Comment #180
timmillwoodSorry @hchonov, I know you don't agree with this approach, but it works!
Comment #181
plachFrom #2891215-17: Add a way to track whether a revision was default when originally created :)
Comment #182
plach@timmillwood is exploring the possibility to create drafts for various translations without the limitations introduced here in #2860097: Ensure that content translations can be moderated independently.
Comment #183
plachThis is marked as MUST-HAVE in the roadmap.
Comment #184
timmillwoodOn the triage call yesterday we discussed that this issue should be committed ASAP and we can work on #2860097: Ensure that content translations can be moderated independently over the coming weeks or months.
Please review!
Comment #185
plachI manually tested this and it's working as intended, however it's still possible to end-up in the problematic situation described in the OP by using the moderation state selector in the node view page:
Should we update the comment too?
This is creating a URL always having the default entity language, whereas we need the latest RTA language.
We can use
$moderation_info
here.Shouldn't we do this only when the moderation state is changing to compensate for the fact that the CMS field is computed?
We can user
$this->t()
here.Is this still needed? If so we should add a check that this is non-default revision, since the one translation-affected per revision constraint applies only to those. For default revisions we should probably return FALSE or throw an exception. Also, the PHP docs are incomplete.
Why isn't this part of the
ModerationLocaleTest
class?We should use URL
$options
to specify language.In addition to this, it would be useful to publish the FR draft and create an English one, just in case.
Why are we removing these assertions?
Comment #186
timmillwoodOooh... good spot about the moderation form. Will come back to that, for now addressing all (but one) of the other points raised.
#185.1 - Updated
#185.2 - Todo: Need to think about this further.
#185.3 - Done
#185.4 - No, because the moderation state might not've changed, we might've gone from draft to draft, therefore all translations in the forward revision will be RTA NULL.
#185.5 - Done
#185.6 - Doesn't look like this is used anymore, so maybe we don't need it.
#185.7 - I guess it could, but as it was testing changes to a form this seemed a ModerationFormTest seemed a good place. Should we move it?
#185.8 - Done
#185.9 - Done
#185.10 - These can go back.
Comment #187
plach@timmillwood:
Can you remind me again why that's a problem? I thought we concluded that we could just skip those no RTA revisions while looking up for the correct RTA revision.
Nope, that makes sense, thanks :)
Was this removed because that was problematic for the following assertions, or was it simply redundant?
We're not asserting that the actions were actually successful :)
Setting to NW for the missing items of #185.
Comment #188
timmillwoodWe don't know which is the "forward revision" translation. We could look for the correct RTA revision, but for which translation?
#187.1 - This was just moved below the english check. So instead of check french, check english, finish test. It is now, check english, check french, add french, add english, finish test. Make sense?
#187.2 - This patch doesn't really prevent them from being successful, but it removes the buttons, hence why so many asserts for the buttons.
Comment #189
timmillwoodFinal bits from #185.
- Loading the RTA translation from a revision for the links in the error message.
- Adding the same logic to the Moderation form to prevent it from showing.
I guess both of these things needs tests.
Comment #190
timmillwoodComment #191
timmillwoodHere's the tests for #189.
Comment #192
plachLooks great and works as intended, however I still have a couple of minor remarks, sorry :)
I was wondering: would it make sense to add also a validation constraint so we can deal also with programmatic use cases and accidental form submissions if buttons are somehow restored?
Maybe we could simply call this
::getTranslationAffectedRevision()
? Anyway, as mentioned above we should check this is a non-default revision and return FALSE or throw an exception if it is default, because in this case the concept makes no sense.Can we move this logic to an (internal?) API method?
Comment #193
plachSorry, I meant
::getAffectedRevisionTranslation()
:)Comment #194
timmillwoodFixing #192 1 & 2.
Comment #195
plachAwesome, last two nits and we're good to go!
We agreed on adding a @todo to remove this in #2860097: Ensure that content translations can be moderated independently :)
I guess this could be marked as @internal, since we're likely going to remove it in #2860097: Ensure that content translations can be moderated independently.
Comment #197
timmillwoodFixing failing tests and points from #195.
Comment #198
plachCool, let's get this in
Comment #199
plachseriously now
Comment #200
catchNitpick but drupal_set_message() should be called with a single string, even if we have to build the string again because the message and label are used elsewhere, otherwise the two strings concatenated may not work for RTL languages.
Otherwise looks good, and self-contained to content_moderation so we can iterate in the other issue.
Comment #201
plachOn that
Comment #202
plachAddressed #200.
Comment #203
timmillwood+1
Comment #205
catchCommitted 75fdbf0 and pushed to 8.4.x. Thanks!
Fixed these on commit:
Comment #206
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedSince moderation can't be done properly on revisionable and translatable entity types without this 'revision_translation_affected' field, I opened a followup to discuss how to provide it generically: #2896845: Provide the 'revision_translation_affected' base field by default for all revisionable and translatable entity types