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.

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

Files: 

Comments

alexpott created an issue. See original summary.

hchonov’s picture

Please 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.

timmillwood’s picture

Here's a video where I reproduce the issue: https://www.youtube.com/watch?v=TnzV8eVtmLo

Gábor Hojtsy’s picture

Note that the problem in the video is (as explained by @timmillwood in IRC):

english unpublished added first, then french published, next a french unpublished is created by the default revision is still the published one, then the english is published, thus copying the latest french revision and making it default, unpublishing the french version of the node.
timmillwood’s picture

Status: Active » Needs review
FileSize
2.93 KB

I'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?

alexpott’s picture

This has been spotted in the workbench_moderation queue.

hchonov’s picture

@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.

timmillwood’s picture

But in my video #3 it behaves differently.

Currently looking into if it's content moderation module causing the issue.

timmillwood’s picture

FileSize
1.85 KB

This 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.

timmillwood’s picture

Status: Needs review » Needs work

Recorded 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?

timmillwood’s picture

Component: entity system » content_moderation.module

Pretty 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.

timmillwood’s picture

I think if $latest_revision->isRevisionTranslationAffected() we should return the latest revision, otherwise the default revision?

timmillwood’s picture

Status: Needs work » Needs review

Here's what I'm thinking:

diff --git a/src/ParamConverter/EntityRevisionConverter.php b/src/ParamConverter/EntityRevisionConverter.php
index ee7b2d5..6b97d99 100644
--- a/src/ParamConverter/EntityRevisionConverter.php
+++ b/src/ParamConverter/EntityRevisionConverter.php
@@ -90,8 +90,10 @@ class EntityRevisionConverter extends EntityConverter {

     if ($entity && $this->moderationInformation->isModeratableEntity($entity) && !$this->moderationInformation->isLatestRevision($entity)) {
       $entity_type_id = $this->getEntityTypeFromDefaults($definition, $name, $defaults);
-      $entity = $this->moderationInformation->getLatestRevision($entity_type_id, $value);
-
+      $latest_revision = $this->moderationInformation->getLatestRevision($entity_type_id, $value);
+      if ($latest_revision->isRevisionTranslationAffected()) {
+        $entity = $latest_revision;
+      }
       // If the entity type is translatable, ensure we return the proper
       // translation object for the current context.
       if ($entity instanceof EntityInterface && $entity instanceof TranslatableInterface) {

Going to commit to https://github.com/timmillwood/content_moderation

catch’s picture

Title: A node's default revision is not per translation but the status (aka published state) is » Translations from default lost/skipped when publishing forward revision of a different translation

Trying a re-title to describe the bug here.

james_kerrigan’s picture

Hi!
Just want to note that using approach mentioned in #13 you'll probably loose the french draft after publishing english version.

timmillwood’s picture

@james_kerrigan it's still there, but ends up becoming a past revision, instead of a forward revision, which is a bit strange.

james_kerrigan’s picture

@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

james_kerrigan’s picture

Sorry 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:


  /**
   * {@inheritdoc}
   */
  public function convert($value, $definition, $name, array $defaults) {
    $entity = parent::convert($value, $definition, $name, $defaults);
    // @todo inject this.
    $tracker = \Drupal::service('workbench_moderation.revision_tracker');

    if ($entity && $this->moderationInformation->isModeratableEntity($entity)) {
      $entity_type_id = $this->getEntityTypeFromDefaults($definition, $name, $defaults);
      $latest_revision_id = $tracker->getLatestRevision($entity_type_id, $entity->id(), $entity->language()->getId());
      if ($latest_revision_id) {
        $entity = node_revision_load($latest_revision_id);
      }
      // If the entity type is translatable, ensure we return the proper
      // translation object for the current context.
      if ($entity instanceof EntityInterface && $entity instanceof TranslatableInterface) {
        $entity = $this->entityManager->getTranslationFromContext($entity, NULL, array('operation' => 'entity_upcast'));
      }
    }

    return $entity;
  }

And in the RevisionTracker class:

  /**
   * @param $entity_type
   * @param $entity_id
   * @param $langcode
   * @return mixed
   */
  public function getLatestRevision($entity_type, $entity_id, $langcode) {
    return $this->connection->select($this->tableName, 'tracker')
      ->condition('tracker.entity_type', $entity_type)
      ->condition('tracker.entity_id', $entity_id)
      ->condition('tracker.langcode', $langcode)
      ->fields('tracker', array('revision_id'))
      ->execute()
      ->fetchField();
  }

I can provide a patch if this makes any sense.

timmillwood’s picture

@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.

Gábor Hojtsy’s picture

Hm, if we need to load different revisions based on language, how does that resolve it?

timmillwood’s picture

@gabor I don't know how to explain in text form, this whole issue has damaged my brain.

Gábor Hojtsy’s picture

@timmillwood: can you explain it in spoken words? Maybe talk tomorrow then?

timmillwood’s picture

@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.

Gábor Hojtsy’s picture

But loading for editing is not the problem is it? The problem is what gets saved back then, which is what James says in #15.

timmillwood’s picture

The 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.

james_kerrigan’s picture

@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.

Gábor Hojtsy’s picture

Right, 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.

timmillwood’s picture

@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.

catch’s picture

timmillwood’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
timmillwood’s picture

Merged @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.

Berdir’s picture

I'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.

Gábor Hojtsy’s picture

Re #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.

Gábor Hojtsy’s picture

Not 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.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
64.12 KB

I created a figure about this to help visualize. Am I getting it all right? :)

Gábor Hojtsy’s picture

@timmillwood: so that said,

  1. when you want to edit an entity which does not have a forward revision, loading and saving back its default revision sounds like the best.
  2. if it has a forward revision in a language other than the one you are editing, then saving from the default revision may invalidate those forward revisions depending on how you track forward revisions (can they be in the past?)
  3. it it has a forward revision for the language you are editing, you need to load that forward revision, in which case the status of all the rest of the languages is undefined, they may or may not be in the future or the past
catch’s picture

So 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.

Gábor Hojtsy’s picture

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

What 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(?)

catch’s picture

#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.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
63.14 KB

Updated figure wording as "draft revision" instead of "forward revision" based on feedback from @catch:

catch’s picture

We 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.

Gábor Hojtsy’s picture

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.

I 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.

timmillwood’s picture

The problem with the 'alternative future' in #40 is you now have that pending French revision left hanging.

I 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.

alexpott’s picture

Discussed 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.

Gábor Hojtsy’s picture

Well, 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 :)

timmillwood’s picture

In #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.

chanchal2002’s picture

Getting this issue in 8.1.x

alexpott’s picture

alexpott’s picture

@chanchal2002 I guess you have workbench_moderation installed? Or do you have another way of causing this?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

Status: Needs review » Needs work

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dawehner’s picture

In 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.

dawehner’s picture

On 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.

alexpott’s picture

Priority: Major » Critical
Issue tags: -WI critical

Discussed 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.

timmillwood’s picture

Title: Translations from default lost/skipped when publishing forward revision of a different translation » Forward revisions can become past revisions when working with translations
Issue summary: View changes

The 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: WI: Add revision_parents 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.

timmillwood’s picture

FileSize
24.84 KB

If we were to have revision parents and revision graphs with the current implementation of Content moderation we'd have something like this.

  1. Create an english unpublished node
  2. Create a french translation of the node and publish it, this copies the english revision 1
  3. Create an unpublished french revision, because it's unpublished the default revision will still be 2, the english gets copied again
  4. Publish the english revision, the default french revision will get copied, thus creating a conflict, two revisions (3 & 4) stemming from the same parent (2)

This shows that revision parents and revision graphs will not solve the issue, but might help manage the data.

catch’s picture

Title: Forward revisions can become past revisions when working with translations » Forward revisions + translation UI can result in forked draft revisions

I'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).

timmillwood’s picture

@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.

timmillwood’s picture

As 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?

timmillwood’s picture

Not sure #58 will work, trying to prove it.

timmillwood’s picture

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.

If we do this we will end up with the same issue described in #3 which was resolved prior to Content Moderation's initial commit.

timmillwood’s picture

    en  |  fr
1    X      1
2    0      1
3    1      1
4    1      0
5    1      1

Let's take this example, revisions 1-5, 0==unpublished, 1==published, X==no revision created yet.

  1. We created revision 1 with a published french node, this is therefore the default revision.
  2. Then add a english translation as unpublished, this gets made the default revision.
  3. The english is then published, so revision 3 becomes the default revision.
  4. For revision 4 we create an unpublished forward (draft) revision of the french translation.
  5. Here we prevent a forward revision of english to be created until the french one is published.

Therefore the solution I'm suggesting is enforcing only one language to have forward revisions at any given time.

dawehner’s picture

Therefore the solution I'm suggesting is enforcing only one language to have forward revisions at any given time.

I'm sorry, but this doesn't work on any complex workflow. This basically makes workflow monolingual.

catch’s picture

@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.

timmillwood’s picture

The 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).

timmillwood’s picture

Initial 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.

timmillwood’s picture

Status: Needs work » Needs review
timmillwood’s picture

Because the patch in #63 is form related it'd be great to get #2753717: [PP-1] Add select field to choose moderation state on entity forms committed first, anyone wanna review?

catch’s picture

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.

So theoretically at least this should allow for multiple translators to work together, i.e.:

 - Published original English version.

   - draft revision of French translation
     - draft revision of Spanish translation
        - draft revision of Korean translation
   - French translation published (new default, with unpublished Spanish and Korean translations)
      - Korean translation updated draft revision
   - Korean translation published (new default)

What it doesn't allow is this:

 - Published original English version
   - draft revision of French translation
   - draft revision of Spanish translation
   - draft revision of Korean translation
  - French revision published (new default (Spanish and Korean translation drafts 'orphaned'))
  - Spanish revision published (new default (French translation removed))

@dawehner does that help clarify the exact limitation in regards to #64?

dawehner’s picture

@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?

timmillwood’s picture

While 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.

Status: Needs review » Needs work

The last submitted patch, 67: 2766957-67.patch, failed testing.

catch’s picture

@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:

 - English, French and Korean translations, all published
   - new draft revision editing Korean translation
   - new draft revision editing French translation

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?

timmillwood’s picture

I can't think of a good solution, apart from preventing any entity saves. Maybe if I explain it'll help.

   en | fr
1  0   x
2  0   1
3  0   0
  1. Create an unpublished english revision.
  2. Add a published french revision, so 2 is the default revision.
  3. Add a french forward revision.

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.

catch’s picture

I think the only solution would be allow no editing of the default revision if there are forward revisions.

Yes, 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.

timmillwood’s picture

I 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.

dawehner’s picture

This is the particular workflow/bug I have in mind.

 - Published original English version.
 - Published original Spanish version.
 - Published original Koren version.

   - draft revision of French translation
     - draft revision of Spanish translation
        - draft revision of Korean translation
   - French translation published (Now Spanish and Koren return a 404)

Please ask, if this is not clear.

catch’s picture

No that's clear, to keep linear history, you could do this though:

- Published original English version.
 - Published original Spanish version.
 - Published original Korean version.

   - draft revision of French translation
     - draft revision of Spanish translation
        - draft revision of Korean translation
   - French + Spanish + Korean translation published
timmillwood’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.37 KB
5.64 KB

Here'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.

Status: Needs review » Needs work

The last submitted patch, 80: 2766957-80.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
4.83 KB
9.37 KB

Another progress patch.

Status: Needs review » Needs work

The last submitted patch, 82: 2766957-82.patch, failed testing.

Gábor Hojtsy’s picture

Issue tags: +D8MI, +sprint, +language-content

Yay, 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:

+++ b/core/modules/content_moderation/src/StateTransitionValidation.php
@@ -42,8 +42,10 @@ public function getValidTransitions(ContentEntityInterface $entity, AccountInter
+        && !(count($entity->getTranslationLanguages()) > 1
+        && !($entity->isDefaultRevision() || $entity->isRevisionTranslationAffected()));

I was quite confused pairing up the parenthesis here for a bit. Can you indent this according to the logical grouping happening?

timmillwood’s picture

What 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.

Gábor Hojtsy’s picture

Well, if we would already be dealing with #2753717: [PP-1] 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: [PP-1] 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.

timmillwood’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing
FileSize
4.28 KB
11.97 KB

Yay, tests pass, but still not 100% confident in my logic.

timmillwood’s picture

As 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.

   en  |  fr
1  1      1
2  0      1

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?

timmillwood’s picture

FileSize
3.91 KB
13.98 KB

So 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?

timmillwood’s picture

We 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:

  • Use Archive transition from Editorial workflow workflow.
  • Use Create New Draft transition from Editorial workflow workflow.
  • Use Publish transition from Editorial workflow workflow.
  • Use Restore transition from Editorial workflow workflow.
  • Use Restore to Draft transition from Editorial workflow workflow.
hass’s picture

You cannot publish this content because there is a draft revision in another language which needs publishing first

Really? 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.

timmillwood’s picture

@hass - I'm open to better solutions, and this restriction only comes into force if a data loss would occur from the save.

catch’s picture

Issue tags: +Triaged D8 critical

Discussed 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.

AMDandy’s picture

When 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.

timmillwood’s picture

@AMDandy - This patch prevents you from having multiple revisions in draft state.

AMDandy’s picture

@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?

timmillwood’s picture

FileSize
15.43 KB
24.53 KB
106.84 KB

Adding "some kind of UI affordance to tell people why they cannot do certain things".

dawehner’s picture

Maybe suggesting people to publish the content or revert the draft would be helpful?

dawehner’s picture

-

dawehner’s picture

-

Status: Needs review » Needs work

The last submitted patch, 97: 2766957-97.patch, failed testing.

Bojhan’s picture

Yhea, I prefer to always add a "How to resolve this" part to a error. Not just claim whats wrong.

hass’s picture

I'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.

dawehner’s picture

@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.

timmillwood’s picture

This 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.

timmillwood’s picture

Issue summary: View changes
timmillwood’s picture

Status: Needs work » Needs review
FileSize
69.81 KB
2.15 KB
23.43 KB

Updated the wording, also removed unrelated changes to ModerationInformation.

Edit: embeded the wrong image.

Status: Needs review » Needs work

The last submitted patch, 108: 2766957-108.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review

Ooops, 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.

hass’s picture

The 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.

timmillwood’s picture

Good suggestion @hass, thanks, I'll add that in the next patch.

dixon_’s picture

Issue tags: +Needs usability review
timmillwood’s picture

This 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.

Status: Needs review » Needs work

The last submitted patch, 114: 2766957-114.patch, failed testing.

hass’s picture

$labels = array_map(function ($transition) { return strtolower($transition->label()); }, $invalid_transitions);

I 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. :-)

gaurav.kapoor’s picture

Status: Needs work » Needs review
FileSize
26.29 KB
1001 bytes
hass’s picture

Use %placeholder or someone may add the lowercasing back... :-(

Status: Needs review » Needs work

The last submitted patch, 117: 2766957-117.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
7.21 KB
26.65 KB

Fixing #110 #111 #116 #118. Also removed all form elements if there is no valid transitions.

yoroy’s picture

Status: Needs review » Needs work
Issue tags: -Needs usability review +Usability

This 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.

timmillwood’s picture

Yes, 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?

timmillwood’s picture

Title: Forward revisions + translation UI can result in forked draft revisions » [PP-1] Forward revisions + translation UI can result in forked draft revisions
Status: Needs work » Postponed

Postponing 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.

timmillwood’s picture

Status: Postponed » Needs review
FileSize
6.78 KB
28.81 KB
34.51 KB

Adding 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.

timmillwood’s picture

Status: Needs review » Postponed
timmillwood’s picture

The last submitted patch, 126: 2766957-126.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 126: 2766957-126-combined.patch, failed testing.

timmillwood’s picture

The 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".

timmillwood’s picture

It 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.

timmillwood’s picture

I 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.

Sam152’s picture

FileSize
1.75 KB

This 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.

timmillwood’s picture

Title: [PP-1] Forward revisions + translation UI can result in forked draft revisions » Forward revisions + translation UI can result in forked draft revisions
timmillwood’s picture

Status: Needs work » Needs review
FileSize
7.05 KB
27.74 KB

Coming 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.