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

  1. Enable Content Translation and Content Moderation modules
  2. Add french language on languages page (i.e. /admin/config/regional/language)
  3. Enable translation for article node type on content translation page (i.e. /admin/config/regional/content-language)
  4. Enable "editorial workflow" for article node type on workflow's edit page(i.e. /admin/config/workflow/workflows/manage/editorial)
  5. Create an article in english and publish ('Save and Publish' button)
  6. Translate to french and publish ('Save and Publish' button)
  7. Go to french version of the article. Make changes and save as draft ('save and Create New Draft (this translation)' button)
  8. 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

CommentFileSizeAuthor
#202 interdiff.txt2.18 KBplach
#202 2766957-202.patch32.63 KBplach
#197 2766957-197.patch32.39 KBtimmillwood
#197 interdiff-2766957-197.txt2.98 KBtimmillwood
#194 2766957-194.patch32.24 KBtimmillwood
#194 interdiff-2766957-194.txt5.52 KBtimmillwood
#191 2766957-191.patch32.06 KBtimmillwood
#191 interdiff-2766957-191.txt6.11 KBtimmillwood
#189 2766957-189.patch30.34 KBtimmillwood
#189 interdiff-2766957-189.txt4.57 KBtimmillwood
#186 2766957-186.patch27.34 KBtimmillwood
#186 interdiff-2766957-186.txt12.05 KBtimmillwood
#185 Test_1___Drupal_8_x 2.png123.36 KBplach
#180 2766957-180.patch28.21 KBtimmillwood
#180 interdiff-2766957-180.txt5.25 KBtimmillwood
#178 interdiff-2766957-178.txt7.42 KBtimmillwood
#168 2766957-168.patch27.13 KBtimmillwood
#168 interdiff-2766957-168.txt7.79 KBtimmillwood
#168 Screenshot from 2017-06-22 16-18-28.png116.31 KBtimmillwood
#167 2766957-167.patch24.83 KBtimmillwood
#161 2766957-161.patch24.14 KBtimmillwood
#159 2766957-159.patch1.19 MBtimmillwood
#159 interdiff-2766957-159.txt5.32 KBtimmillwood
#155 scenario-4-french-english-english-french.png228.64 KBvijaycs85
#155 scenario-3-french-english-french-english.png230.34 KBvijaycs85
#155 scenario-2-english-french-french-english.png278.9 KBvijaycs85
#155 scenario-1-english-french-english-french.png214.45 KBvijaycs85
#154 2766957-154.patch22.41 KBtimmillwood
#154 interdiff-2766957-154.txt3.5 KBtimmillwood
#154 Screenshot from 2017-05-15 17-06-24.png105.5 KBtimmillwood
#153 Screenshot from 2017-05-15 16-37-41.png104.11 KBtimmillwood
#152 2766957-152.patch27.65 KBvijaycs85
#152 2766957-diff-148-152.txt656 bytesvijaycs85
#148 2766957-diff-147-148.txt5.15 KBvijaycs85
#148 2766957-148.patch27.35 KBvijaycs85
#148 two-drafts.png104.69 KBvijaycs85
#147 2766957-146.patch24.03 KBvijaycs85
#147 two-forward-revisions.png101.61 KBvijaycs85
#147 no-english-revision.png96.57 KBvijaycs85
#141 2766957-141.patch23.92 KBtimmillwood
#141 interdiff-2766957-141.txt757 bytestimmillwood
#139 2766957-139.patch24.37 KBtimmillwood
#139 interdiff-2766957-139.txt11.44 KBtimmillwood
#134 2766957-134.patch27.74 KBtimmillwood
#134 interdiff-2766957-134.txt7.05 KBtimmillwood
#132 2766957-132.patch1.75 KBSam152
#126 2766957-126-combined.patch35.75 KBtimmillwood
#126 2766957-126.patch28.95 KBtimmillwood
#126 interdiff-2766957-126.txt2.79 KBtimmillwood
#124 2766957-124-combined.patch34.51 KBtimmillwood
#124 2766957-124.patch28.81 KBtimmillwood
#124 interdiff.txt6.78 KBtimmillwood
#120 2766957-120.patch26.65 KBtimmillwood
#120 interdiff-2766957-120.txt7.21 KBtimmillwood
#117 interdiff-114-117.txt1001 bytesgaurav.kapoor
#117 2766957-117.patch26.29 KBgaurav.kapoor
#114 Screenshot from 2017-03-15 17-29-55.png49.58 KBtimmillwood
#114 2766957-114.patch26.3 KBtimmillwood
#114 interdiff-2766957-114.txt5.16 KBtimmillwood
#108 2766957-108.patch23.43 KBtimmillwood
#108 interdiff.txt2.15 KBtimmillwood
#108 Screenshot from 2017-03-13 11-35-46.png69.81 KBtimmillwood
#97 Screenshot from 2017-03-10 17-22-57.png106.84 KBtimmillwood
#97 2766957-97.patch24.53 KBtimmillwood
#97 interdiff.txt15.43 KBtimmillwood
#89 2766957-89.patch13.98 KBtimmillwood
#89 interdiff.txt3.91 KBtimmillwood
#87 2766957-87.patch11.97 KBtimmillwood
#87 interdiff.txt4.28 KBtimmillwood
#82 2766957-82.patch9.37 KBtimmillwood
#82 interdiff.txt4.83 KBtimmillwood
#80 2766957-80.patch5.64 KBtimmillwood
#80 interdiff.txt5.37 KBtimmillwood
#67 2766957-67.patch1.21 KBtimmillwood
#57 Untitled Diagram.png24.84 KBtimmillwood
#40 Revisions vs. language (1).jpg63.14 KBGábor Hojtsy
#35 Revisions vs. language.jpg64.12 KBGábor Hojtsy
#9 replicating.txt1.85 KBtimmillwood
#5 a_node_s_default-2766957-5.patch2.93 KBtimmillwood
Support from Acquia helps fund testing for Drupal Acquia logo

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.

artem_sylchuk’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.

artem_sylchuk’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

artem_sylchuk’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.

artem_sylchuk’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: 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.

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

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

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.

dixon_’s picture

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

AMDandy’s picture

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

timmillwood’s picture

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

AMDandy’s picture

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

timmillwood’s picture

FileSize
11.44 KB
24.37 KB

Trying to simplify the patch.

Status: Needs review » Needs work

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

timmillwood’s picture

Status: Needs work » Needs review
FileSize
757 bytes
23.92 KB

Forgot to remove a hook.

Status: Needs review » Needs work

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

catch’s picture

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

tacituseu’s picture

Status: Needs work » Needs review

Unrelated failure.

timmillwood’s picture

Still looking for reviews and manual testing of this.

timmillwood’s picture

Issue tags: +Needs screenshots

It'd be great to get some screenshots of the output given in #141 when someone does manual testing.

  1. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -353,12 +364,37 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +          drupal_set_message('A draft revision in another translation is preventing this from being saved.', 'error');
    

    I wonder if it's worth putting the "publish or delete" links in the DSM.

  2. +++ b/core/modules/content_moderation/src/StateTransitionValidation.php
    @@ -47,4 +47,4 @@ public function getValidTransitions(ContentEntityInterface $entity, AccountInter
    \ No newline at end of file
    
    +++ b/core/modules/content_moderation/src/StateTransitionValidationInterface.php
    @@ -23,4 +23,4 @@
    \ No newline at end of file
    

    These need addressing too.

vijaycs85’s picture

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

vijaycs85’s picture

Issue summary: View changes
FileSize
104.69 KB
27.35 KB
5.15 KB

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

The last submitted patch, 147: 2766957-146.patch, failed testing.

Status: Needs review » Needs work

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

catch’s picture

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

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
656 bytes
27.65 KB

Fixing test fails. timmillwood++

timmillwood’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
104.11 KB

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

timmillwood’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs screenshots
FileSize
105.5 KB
3.5 KB
22.41 KB

Fixing #146 and #153.

vijaycs85’s picture

Thanks for the update and fixing the settings file mess @timmillwood. I just tested the patch in #154 and looks working as expected.

Revision ID Original language Status Translated language Status Error displayed language
Scenario 1
1 English published French published -
2 English draft French can't save French
Scenario 2
1 English published French published -
2 French draft English can't save English
Scenario 3
1 French published English published -
2 French draft English can't save English
Scenario 4
1 French published English published -
2 English draft French can't save French

Attached screenshot for all 4 scenarios.

timmillwood’s picture

Thank @vijaycs85, glad it's working for you.

catch’s picture

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

  1. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -282,12 +292,37 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
               ->enforceRevisionsEntityFormAlter($form, $form_state, $form_id);
    +
    +        if ((!$entity->isRevisionTranslationAffected() || !$this->moderationInfo->isLatestRevision($entity)) && count($entity->getTranslationLanguages()) > 1 && $this->moderationInfo->hasForwardRevision($entity)) {
    +          $latest_revision = $this->getTranslationAffectedRevision($this->moderationInfo->getLatestRevision($entity->getEntityTypeId(), $entity->id()));
    

    This could use a summary of the logic inline as well as why we do it.

  2. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -282,12 +292,37 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +          drupal_set_message(\Drupal::translation()->translate('It is not possible to save this @entity_type_label, <a href="@latest_revision_edit_url">publish</a> or <a href="@latest_revision_delete_url">delete</a> the latest revision to allow all workflow transitions.', ['@entity_type_label' => $entity->getEntityType()->getLabel(), '@latest_revision_edit_url' => $latest_revision->toUrl('edit-form', ['language' => $latest_revision->language()])->toString(), '@latest_revision_delete_url' => $latest_revision->toUrl('delete-form', ['language' => $latest_revision->language()])->toString()]), 'error');
    

    s/latest revision/draft revision/g?

  3. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
    @@ -183,4 +196,151 @@ public function testNonBundleModerationForm() {
    +
    +    // Make sure we're allowed to create a forward french revision.
    +    $this->drupalGet('fr/' . $edit_path);
    +    $this->assertTrue($this->xpath('//input[@value="Save and Create New Draft (this translation)"]'));
    +    $this->assertTrue($this->xpath('//input[@value="Save and Publish (this translation)"]'));
    +    $this->assertTrue($this->xpath('//input[@value="Save and Archive (this translation)"]'));
    +
    +    // Add a english forward revision (revision 6).
    +    $this->drupalGet($edit_path);
    +    $this->assertTrue($this->xpath('//input[@value="Save and Create New Draft (this translation)"]'));
    +    $this->assertTrue($this->xpath('//input[@value="Save and Publish (this translation)"]'));
    +    $this->assertTrue($this->xpath('//input[@value="Save and Archive (this translation)"]'));
    +    $this->drupalPostForm(NULL, [
    +      'body[0][value]' => 'Seventh version of the content.',
    +    ], t('Save and Create New Draft (this translation)'));
    +
    +    // Make sure we're not allowed to create a forward french revision.
    +    $this->drupalGet('fr/' . $edit_path);
    +    $this->assertFalse($this->xpath('//input[@value="Save and Create New Draft (this translation)"]'));
    +    $this->assertFalse($this->xpath('//input[@value="Save and Publish (this translation)"]'));
    +    $this->assertFalse($this->xpath('//input[@value="Save and Archive (this translation)"]'));
    +    $this->assertText('It is not possible to save this Content.');
    +
    

    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.

timmillwood’s picture

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

timmillwood’s picture

FileSize
5.32 KB
1.19 MB

This patch fixes #158.1 and #158.2, it also fixes some things that go lost in a re-roll.

Status: Needs review » Needs work

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

timmillwood’s picture

Status: Needs work » Needs review
FileSize
24.14 KB

Helps if I create the patch against 8.4.x.

Interdiff from #159 is still correct.

catch’s picture

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.

So yes definitely this I think would remove some of the limitations this patch adds - we're only enforcing a linear version history here.

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.

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?

timmillwood’s picture

The linear version history is resolved in #161.

I agree "Save and Publish (All translations)" should be a follow up.

vijaycs85’s picture

Bojhan’s picture

Can we move from "It is not possible to save this Content." to "Unable to save this Article." ? We should have singular forms of entities?

Status: Needs review » Needs work

The last submitted patch, 161: 2766957-161.patch, failed testing. View results

timmillwood’s picture

Status: Needs work » Needs review
FileSize
24.83 KB

Re-roll of #161.

timmillwood’s picture

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

Status: Needs review » Needs work

The last submitted patch, 168: 2766957-168.patch, failed testing. View results

timmillwood’s picture

Status: Needs work » Postponed

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

timmillwood’s picture

Status: Postponed » Needs work

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

xjm’s picture

plach’s picture

Yesterday I spoke about the approach outlined in #170 with @catch. In general he was onboard with the approach we discussed. We agreed that:

  • Strict constraints on supported use cases are desirable: this is an area the can quickly become hugely complex. Content Moderation should target the most common use cases that allow to satisfy the majority of users. Everything else should be dealt with in contrib.
  • As mentioned in #170, the main assumption is that we will have only one translation changed for each revision. This allows to present users with a different latest revision depending on the current language, via the 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.
  • We should keep track of the historical state of a revision, e.g. if it was a default revision or a draft one. This will help presenting users with meaningful diffs. The current default revision is the one referenced in the entity base table, e.g. {node}.
  • It could be desirable to implement an opt-in garbage collector that would get rid of merged drafts, i.e. drafts that have a newer default revision affecting the same language.

@catch noted that probably in core we don't need a revision_type field, in fact a draft 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.

timmillwood’s picture

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

catch’s picture

This allows to present users with a different latest revision depending on the current language, via the revision_translation_affected field.

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

plach’s picture

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

timmillwood’s picture

+++ b/core/modules/content_moderation/src/ParamConverter/EntityRevisionConverter.php
@@ -92,13 +92,12 @@ public function convert($value, $definition, $name, array $defaults) {
-      // If the entity type is translatable, ensure we return the proper
-      // translation object for the current context.
-      if ($latest_revision instanceof EntityInterface && $entity instanceof TranslatableInterface) {
-        $latest_revision = $this->entityManager->getTranslationFromContext($latest_revision, NULL, ['operation' => 'entity_upcast']);
-      }
-
-      if ($latest_revision instanceof EntityInterface && $latest_revision->isRevisionTranslationAffected()) {
+      if ($latest_revision instanceof EntityInterface) {
+        // If the entity type is translatable, ensure we return the proper
+        // translation object for the current context.
+        if ($entity instanceof TranslatableInterface) {
+          $latest_revision = $this->entityManager->getTranslationFromContext($latest_revision, NULL, ['operation' => 'entity_upcast']);
+        }

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

timmillwood’s picture

FileSize
7.42 KB

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

hchonov’s picture

From #2891215-16: Add a way to track whether a revision was default when originally created:

I would like to mention it again that I am not really convinced by setting that flag manually to TRUE, because the idea of this flag was that it is computed based if there are real editorial changes on the content fields of a translation. Setting it manually in order to spare us some work on introducing a revision_type feels not really right. I am pretty sure that a revision_type field might and will be really useful. The idea is that even if you solve it through setting the field manually how could one say if a previous revision was a default or a draft one? Out there are systems where the editorial history is important.

timmillwood’s picture

Sorry @hchonov, I know you don't agree with this approach, but it works!

plach’s picture

From #2891215-17: Add a way to track whether a revision was default when originally created :)

Well, in my mind manually setting the RTA flag is not about not doing the revision_type change, it is about not doing the revision_langcode one. As I mentioned in the call, we already have all sorts of language fields and methods scattered through out the Entity Field API. Introducing a "revision language" concept when we support multiple languages per revision would further increase the confusion.

In IRC @timmillwood clarified that the moderation state is actual entity metadata, it's only implemented as a computed field to overcome some technical difficulties. So when you change the moderation state conceptually you are actually changing the entity and we agreed that it would be good if CM integrated with Diff to make this visible in diffs.

plach’s picture

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

plach’s picture

Issue tags: +WI critical

This is marked as MUST-HAVE in the roadmap.

timmillwood’s picture

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

plach’s picture

Status: Needs review » Needs work
FileSize
123.36 KB

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

  1. +++ b/core/modules/content_moderation/content_moderation.module
    @@ -131,34 +131,34 @@ function content_moderation_entity_view(array &$build, EntityInterface $entity,
      * Nodes in particular should be viewable if unpublished and the user has
    

    Should we update the comment too?

  2. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -282,12 +293,53 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +          $message = \Drupal::translation()->translate('<a href="@latest_revision_edit_url">Publish</a> or <a href="@latest_revision_delete_url">delete</a> the latest draft revision to allow all workflow transitions.', ['@latest_revision_edit_url' => $latest_revision->toUrl('edit-form', ['language' => $latest_revision->language()])->toString(), '@latest_revision_delete_url' => $latest_revision->toUrl('delete-form', ['language' => $latest_revision->language()])->toString()]);
    

    This is creating a URL always having the default entity language, whereas we need the latest RTA language.

  3. +++ b/core/modules/content_moderation/content_moderation.module
    @@ -131,34 +131,34 @@ function content_moderation_entity_view(array &$build, EntityInterface $entity,
    -    $workflow = \Drupal::service('content_moderation.moderation_information')->getWorkflowForEntity($node);
    +    $workflow = \Drupal::service('content_moderation.moderation_information')->getWorkflowForEntity($entity);
    

    We can use $moderation_info here.

  4. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
    @@ -33,6 +33,9 @@ public function onPresave(ContentEntityInterface $entity, $default_revision, $pu
    +    if ($entity->hasField('revision_translation_affected')) {
    +      $entity->set('revision_translation_affected', TRUE);
    +    }
    

    Shouldn't we do this only when the moderation state is changing to compensate for the fact that the CMS field is computed?

  5. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -282,12 +293,53 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +          $message = \Drupal::translation()->translate('<a href="@latest_revision_edit_url">Publish</a> or <a href="@latest_revision_delete_url">delete</a> the latest draft revision to allow all workflow transitions.', ['@latest_revision_edit_url' => $latest_revision->toUrl('edit-form', ['language' => $latest_revision->language()])->toString(), '@latest_revision_delete_url' => $latest_revision->toUrl('delete-form', ['language' => $latest_revision->language()])->toString()]);
    

    We can user $this->t() here.

  6. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -295,6 +347,23 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +   * @param \Drupal\Core\Entity\EntityInterface $entity
    +   * @return \Drupal\Core\Entity\EntityInterface
    +   */
    +  protected function getTranslationAffectedRevision(EntityInterface $entity) {
    +    if ($entity->isRevisionTranslationAffected()) {
    

    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.

  7. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
    @@ -205,4 +221,191 @@ public function testModerationFormSetsRevisionAuthor() {
    +  public function testContentTranslationNodeForm() {
    

    Why isn't this part of the ModerationLocaleTest class?

  8. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
    @@ -205,4 +221,191 @@ public function testModerationFormSetsRevisionAuthor() {
    +    // Add french forward revision (revision 3).
    +    $this->drupalGet('fr/' . $edit_path);
    ...
    +    // Publish the french forward revision (revision 4).
    +    $this->drupalGet('fr/' . $edit_path);
    ...
    +    // Make sure we're allowed to create a forward french revision.
    +    $this->drupalGet('fr/' . $edit_path);
    ...
    +    // Make sure we're not allowed to create a forward french revision.
    +    $this->drupalGet('fr/' . $edit_path);
    ...
    +    // Make sure we're allowed to create a forward french revision.
    +    $this->drupalGet('fr/' . $edit_path);
    ...
    +    // Add another draft for the translation (revision 3).
    +    $this->drupalGet('fr/' . $edit_path);
    ...
    +    // It should be possible to create a further draft.
    +    $this->drupalGet('fr/' . $edit_path);
    

    We should use URL $options to specify language.

  9. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
    @@ -205,4 +221,191 @@ public function testModerationFormSetsRevisionAuthor() {
    +    // Editing the original translation should not be possible.
    +    $this->drupalGet($edit_path);
    

    In addition to this, it would be useful to publish the FR draft and create an English one, just in case.

  10. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationLocaleTest.php
    @@ -144,17 +144,12 @@ public function testTranslateModeratedContent() {
    -    $this->assertTrue($french_node->isPublished());
    -    $this->assertEqual($french_node->getTitle(), 'New draft of translated node', 'The draft has replaced the published revision.');
    

    Why are we removing these assertions?

timmillwood’s picture

Status: Needs work » Needs review
FileSize
12.05 KB
27.34 KB

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

plach’s picture

Status: Needs review » Needs work

@timmillwood:

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.

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.

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?

Nope, that makes sense, thanks :)

  1. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
    @@ -388,24 +385,32 @@ public function testContentTranslationNodeForm() {
    -    // It should be possible to create a further draft.
    -    $this->drupalGet('fr/' . $edit_path);
    -    $this->assertTrue($this->xpath('//input[@value="Save and Create New Draft (this translation)"]'));
    -    $this->assertTrue($this->xpath('//input[@value="Save and Publish (this translation)"]'));
    -    $this->assertFalse($this->xpath('//input[@value="Save and Archive (this translation)"]'));
    -
    

    Was this removed because that was problematic for the following assertions, or was it simply redundant?

  2. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
    @@ -388,24 +385,32 @@ public function testContentTranslationNodeForm() {
    +    // Updating and publishing the french translation is still possible.
    +    $this->drupalGet($edit_path, ['language' => $french]);
    +    $this->assertTrue($this->xpath('//input[@value="Save and Create New Draft (this translation)"]'));
    +    $this->assertTrue($this->xpath('//input[@value="Save and Publish (this translation)"]'));
    +    $this->assertFalse($this->xpath('//input[@value="Save and Archive (this translation)"]'));
    +    $this->drupalPostForm(NULL, [], t('Save and Publish (this translation)'));
    +
    +    // Now the french translation is published, an english draft can be added.
    +    $this->drupalGet($edit_path);
    +    $this->assertTrue($this->xpath('//input[@value="Save and Create New Draft (this translation)"]'));
    +    $this->assertTrue($this->xpath('//input[@value="Save and Publish (this translation)"]'));
    +    $this->assertTrue($this->xpath('//input[@value="Save and Archive (this translation)"]'));
    +    $this->drupalPostForm(NULL, [], t('Save and Create New Draft (this translation)'));
    

    We're not asserting that the actions were actually successful :)

Setting to NW for the missing items of #185.

timmillwood’s picture

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.

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

timmillwood’s picture

FileSize
4.57 KB
30.34 KB

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

timmillwood’s picture

Status: Needs work » Needs review
timmillwood’s picture

FileSize
6.11 KB
32.06 KB

Here's the tests for #189.

plach’s picture

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

  1. +++ b/core/modules/content_moderation/src/ModerationInformation.php
    @@ -115,6 +115,18 @@ public function getDefaultRevisionId($entity_type_id, $entity_id) {
    +  public function getRevisionTranslationAffectedTranslation(ContentEntityInterface $entity) {
    
    +++ b/core/modules/content_moderation/src/ModerationInformationInterface.php
    @@ -90,6 +90,17 @@ public function getLatestRevisionId($entity_type_id, $entity_id);
    +   * Returns the revision translation affected translation of a revision.
    +   *
    +   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
    +   *   The content entity.
    +   *
    +   * @return \Drupal\Core\Entity\ContentEntityInterface
    +   *   The revision translation affected translation.
    +   */
    +  public function getRevisionTranslationAffectedTranslation(ContentEntityInterface $entity);
    

    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.

  2. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -282,12 +293,54 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +        if (!$entity->isRevisionTranslationAffected() && count($entity->getTranslationLanguages()) > 1 && $this->moderationInfo->hasForwardRevision($entity)) {
    

    Can we move this logic to an (internal?) API method?

plach’s picture

Maybe we could simply call this ::getTranslationAffectedRevision()?

Sorry, I meant ::getAffectedRevisionTranslation() :)

timmillwood’s picture

plach’s picture

Awesome, last two nits and we're good to go!

  1. +++ b/core/modules/content_moderation/src/Entity/Handler/ModerationHandler.php
    @@ -33,6 +33,9 @@ public function onPresave(ContentEntityInterface $entity, $default_revision, $pu
    +    if ($entity->hasField('revision_translation_affected')) {
    

    We agreed on adding a @todo to remove this in #2860097: Ensure that content translations can be moderated independently :)

  2. +++ b/core/modules/content_moderation/src/ModerationInformationInterface.php
    @@ -98,7 +98,18 @@ public function getDefaultRevisionId($entity_type_id, $entity_id);
    +  public function isForwardRevisionAllowed(ContentEntityInterface $entity);
    

    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.

Status: Needs review » Needs work

The last submitted patch, 194: 2766957-194.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
2.98 KB
32.39 KB

Fixing failing tests and points from #195.

plach’s picture

Cool, let's get this in

plach’s picture

Status: Needs review » Reviewed & tested by the community

seriously now

catch’s picture

+++ b/core/modules/content_moderation/src/EntityTypeInfo.php
@@ -282,12 +293,50 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
+          }
+          $label = $this->t('Unable to save this @type_label.', ['@type_label' => $type_label]);
+          $translation = $this->moderationInfo->getAffectedRevisionTranslation($latest_revision);
+          $message = $this->t('<a href="@latest_revision_edit_url">Publish</a> or <a href="@latest_revision_delete_url">delete</a> the latest draft revision to allow all workflow transitions.', ['@latest_revision_edit_url' => $translation->toUrl('edit-form', ['language' => $translation->language()])->toString(), '@latest_revision_delete_url' => $translation->toUrl('delete-form', ['language' => $translation->language()])->toString()]);
+
+          drupal_set_message(Markup::create($label . ' ' . $message), 'error');

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

plach’s picture

Assigned: Unassigned » plach

On that

plach’s picture

Assigned: plach » Unassigned
FileSize
32.63 KB
2.18 KB

Addressed #200.

timmillwood’s picture

+1

  • catch committed 75fdbf0 on 8.4.x
    Issue #2766957 by timmillwood, vijaycs85, plach, Sam152, Gábor Hojtsy,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 75fdbf0 and pushed to 8.4.x. Thanks!

Fixed these on commit:

FILE: .../8.x/core/modules/content_moderation/src/EntityTypeInfo.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------
 10 | WARNING | [x] Unused use statement
 16 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------
amateescu’s picture

Since 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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.