New translations for moderated nodes are not created in the initial workflow state

This has come up a lot in the queue and was raised by @timmillwood before content moderation hit a beta status.

But shouldn't new translations be treated as new nodes and therefore be created as "draft"?

I feel confident that changing this now would break workflows setup for existing users. To that end, this needs to be marked a feature request and a fix would need to allow for existing users to continue using the system the way it currently works. An alternative may also be to document a code-based way of overriding this, instead of building it into core.

NW to discuss #8 and for tests.

Interesting pattern (twice in one week).

1. ignore a bug report (with patch) for over a year
2. then close that original bug report, claiming that it duplicated a ticket which didn't exist at the time
3. declare that the bug has been around so long that it's no longer a bug
4. change the new report of the bug into a "feature" request

Does @Dries still wonder why it's hard to find enough contributors to open source projects?

Hi @bkline,

This issue examining the starting state of new translations predates the issue you opened as well: #2927455: When adding a new translation the entity should start from the draft state. You could argue all the follow-ups should be closed in favour of that one. Personally I don't mind too much, since this one brings an up-to-date title, summary and steps to reproduce to the table (the customisable initial workflow state wasn't a thing back then), but if anyone feels strongly about that I'd be fine reorganising it.

Re: feature request, my reading of these issues so far is that fixing the problem for some users may negatively impact other users. Anyone can change the category back if they disagree.

Not really interested in responding to your other points, it'd be wise to remember a lot of contributors are volunteers.

Hi @bahuma20,

There are two additional changes that I think need to be made, one easy to explain, one much harder.

Change 1 - Fix the ModerationStateWidget to use the state of the translation
Issue #2914873 added in an additional hoop to this problem. When a new translation is “created,” and the edit form is displayed, the widget gets the workflow state of the as of yet translated item. It calls `$original_entity = $original_entity->getTranslation($entity->language()->getId());` which actually will return the original entity that was being translated. So the same check for isNewTranslation you added to `getOriginalState` needs to be added to this as well. Looking something like:

    // If the entity already exists, grab the most recent revision and load it.
    // The moderation state of the saved revision will be used to display the
    // current state as well determine the appropriate transitions.
    if (!$entity->isNew() && !$entity->isNewTranslation()) {
      /** @var \Drupal\Core\Entity\ContentEntityInterface $original_entity */
      $original_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadRevision($entity->getLoadedRevisionId());
      if (!$entity->isDefaultTranslation() && $original_entity->hasTranslation($entity->language()->getId())) {
        $original_entity = $original_entity->getTranslation($entity->language()->getId());
      }
    }

Change 2 - Fix how moderation_state get set in the first place for a new translation

So one of the major issues with how everything works is that the `moderation_state` field is a calculated field, so it show up just like any normal field. The when a new translation is created, the translation system makes a copy of all the fields of the entity -- including `moderation_state`. This was purposefully done in #2915398 to copy the moderation state from the original item instead of using the workflow default for the translation. So if your original language content has been published, when the translation is created the `moderation_state` “field” is copied into the entity.

So what can be done is to implement a hook for `hook_entity_translation_create()` and change the moderation_state to whatever the workflow default is. You cannot just set the field to NULL and have `getOriginalState` work things out because the `moderation_state` field MUST be set to a valid state. (It probably also makes sense in a way since you would never really want to make that field NULL when an item has entered into a workflow) (getOriginalState could probably be a static method that can be called by this hook and the new node creation)

This would look something like:

/**
 * Implements hook_entity_translation_create().
 */
function MY_MODULE_entity_translation_create(EntityInterface $translation) {
  // We know that EntityStorageBase just copies all fields on a translations,
  // including moderation_state. This is not correct for a new translation,
  // which should start at the default state for the workflow.
  $serviceName = 'content_moderation.moderation_information';
  $moderationInformation = \Drupal::service($serviceName);
  if ($moderationInformation->isModeratedEntity($translation)) {
    $workflow = $moderationInformation->getWorkflowForEntity($translation);
    $workflow_configuration = $workflow->getTypePlugin()->getConfiguration();
    $state = 'draft';
    if (!empty($workflow_configuration['default_moderation_state'])) {
      $state = $workflow_configuration['default_moderation_state'];
    }
    $translation->moderation_state = $state;
  }
}

Possible Better Solution for 2 (still requiring change 1)
Probably the best solution would be to add a flag to the workflow to "Use default workflow state for translations" and then have the `moderation_state` computed field class determine if it should get the state from the original item, or use the default workflow state. (Or it does it by implementing a entity_translation_create hook) This would support the current use case where translation workflow follows the original item. Additionally it would support this use case where translations lag behind the original language, as in the case where a site owner has more "original language" writers than "translated language" writers, but still wants to support other minority populations in their community. In those cases the "original language" could be published weeks before the "translation language" versions as workload allows, while still allowing the full workflow for the "translation language" content.

Hi,

On our side we need the same feature as describe above : a new translation has to be initialize in draft state.
This works well with the patch 3150294-5.patch.

But it remains a problem : the "destination" states proposed in the "Change to" list are not the one authorized for the draft states.
This "Change to" list behave as if the current state was the one of the original node.

This is solved by the "Change 1" describe by @bpizzillo.

Here is a new patch based on 3150294-5.patch + the "Change 1".

only in patch2:
unchanged:
--- a/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
+++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
@@ -125,7 +125,7 @@ class ModerationStateWidget extends OptionsSelectWidget implements ContainerFact
// If the entity already exists, grab the most recent revision and load it.
// The moderation state of the saved revision will be used to display the
// current state as well determine the appropriate transitions.
- if (!$entity->isNew()) {
+ if (!$entity->isNew() && !$entity->isNewTranslation()) {
/** @var \Drupal\Core\Entity\ContentEntityInterface $original_entity */
$original_entity = $this->entityTypeManager->getStorage($entity->getEntityTypeId())->loadRevision($entity->getLoadedRevisionId());
if (!$entity->isDefaultTranslation() && $original_entity->hasTranslation($entity->language()->getId())) {

Re: #5: Thanks for finding, reporting and fixing this with a patch!

Re: #8:
- Still needs tests, so back to NW. Also added to remaining tasks.
- I'm in the middle of assessing how translations will fit into a content_moderation-based site and I hit this bug. Yes, I think it's a bug.
- Yes, some sites might already be working around it. I predict most will expect it work like this and will be surprised and disappointed to find that's not the case.
- I think we will have to fix this in a BC way.
- We shouldn't fix this by documenting custom code people have to add to get it to work as most would expect.

Re: #15 and BC ways to fix this:

I think we should add a new property to workflow definitions related to this, give it the correct default, and have the update hook here set the value to the current behavior for existing sites. Everyone wins, and we store the info close to where we need it, not in an entirely separate storage mechanism.

Re: #16 and transition UI:

Good find. I agree, that's probably in scope here. Starting a new translation should really be more like starting a brand new node. We shouldn't consider this a state transition, we should always allow it to anyone that's allowed to create a new translation. Happy to see this is already in the summary.

Re: #17 and fixes:

1. Yes, thanks.
2. Hrm.
2+. Yes! This is basically what I wrote up before I started this careful review of the issue history. ;) See below.

Re: #19: Thanks for fixing #17.1! Please always include an interdiff with a new patch.

Re: #20: Thanks for posting #19's interdiff. ;)

Some ideas on how the new setting should work, which I'm putting in the summary:

Option A

• Have a simple toggle for the current behavior vs using the default moderation state.
• UI is just a checkbox. Have to word it carefully to be clear.
• Setting is a bool. ID and other details TBD based on UI.

Example UI:

[X] New translations use default moderation state.
(If not checked, new translations will use the current state of the default revision).

(Don't love having the description, but without it, not sure folks would understand the implications of turning this off).

Option B

• Have a simple toggle for the current behavior vs using the default moderation state (same as A)
• UI is a select or radios.
• Setting stores a string, ID would be 'translation_default_moderation_state_behavior' or so, with values like 'current' vs. 'default'.
• In some UIs, explicit radios for different options can be more clear than a checkbox, if each choice requires a little explanation.

Example UI:

Moderation state for new translations:
(*) Default moderation state.
( ) Current moderation state of the default revision.

Option C

• Store the explicit moderation state for new translations.
• Setting ID would be 'translation_default_moderation_state' or so.
• Allow it to have a magic value that means "the current moderation state of the default revision in the original language".
• UI might be weird / complicated.
• Would be more powerful for edge cases.

I think I lean towards B, since I imagine it'll be hard to improve A enough to not need a description. If we can come up with a simple, clear name and behavior that requires no description, A would be better. I'm not attached to my initial attempt above. Improvements / alternatives welcome. ;)

Thanks!
-Derek

We discussed this issue at #3223115: Drupal Usability Meeting 2021-07-16. (Sorry for the delay.) There were only three participants, so a lot fewer points of view than we usually get.

We limited our discussion to the UI options suggested in #22 and the issue summary.

It would help evaluate this issue if you describe the necessary steps in the issue summary:

1. Enable the Content moderation and Content translations modules, and their requirements.
2. Add a language to the site.
3. Enable translation for Content (nodes) and at least one content type.
4. Enable the Editorial workflow for at least one translatable content type.
5. Configure the workflow and/or permissions to expose the problem.

For testing purposes, I think it is possible to have some unrealistic workflow configuration so that even an admin user triggers the error.

Maybe it would be simpler to start with the Umami demo profile. My steps assume the Standard profile.

You did not say in #22 where the new option would be. This context matters! At first, I assumed it would be in the "Language settings" vertical tab of the content type configuration page (such as /admin/structure/types/manage/article). But it makes more sense to put it on the Workflow configuration page, in the same section as the "Default moderation state" select list.

With all that said, I can now answer the question. If it is a boolean option, and if you put it on the Workflow configuration page, then we liked the checkbox option. This seems like the right UI element for an option that modifies how the "Default moderation state" is used. I wish I had a more convincing example, but look at /admin/config/regional/settings: if you allow users to set their own time zones, then the (boolean) option to nag them is exposed as a checkbox.

We did not discuss it at the meeting, but my personal suggestion is to label the checkbox with an imperative statement. For example: "Use the 'Default moderation state' for new translations." or "... when adding a new translation." (I use the same capitalization as in the label: "Default moderation state".)

The imperative and the single quotes are consistent with the Style section of the Interface text page. You should also consider User interface standards.

The issue summary mentions applying a particular patch from #3020387: Moderation state is the same for all node's translations in edit page. That issue has been fixed, with the patch from a later comment. I think the issue summary here should be updated, so I am adding the tag for that.

@benjifisher re: #23 Thanks for reviewing it! Sorry this part wasn't clear:

You did not say in #22 where the new option would be. This context matters! At first, I assumed it would be in the "Language settings" vertical tab of the content type configuration page (such as /admin/structure/types/manage/article). But it makes more sense to put it on the Workflow configuration page, in the same section as the "Default moderation state" select list.

The issue summary already said:

User interface changes

TBD: A new setting when creating/editing a workflow.

Guess that was too short and was easily missed... 😉

It would help evaluate this issue if you describe the necessary steps in the issue summary: ...

Agreed. Fixed. Also made the scenario gender neutral. Hope it's clear enough now for anyone to understand the problem.

If it is a boolean option, and if you put it on the Workflow configuration page, then we liked the checkbox option. This seems like the right UI element for an option that modifies how the "Default moderation state" is used.

I’d rather it be a checkbox, but I think the wording is going to be very tricky to get right. If it just says:

“Use the 'Default moderation state' for new translations.”

I have no idea how anyone would understand what happens if you don’t check the box. If we go with a checkbox, I think we need a #description, too. In that case, what would it say? That’s why I thought radios might be more clear...

Re: #24: Good point. Applying a patch from #3020387 is now an irrelevant detail. Removed. It's still listed as a related issue, if anyone cares.

Also, added this point to the Proposed resolution:

Whatever new UI is introduced should only be visible if both Content moderation and Content translation modules are enabled.

The setting would only impact anything if both modules are enabled, so we shouldn't display it at all on monolingual sites.

With all of the above edits, I think the summary is fine again, so removing the tag. Anyone is welcome to further improve the summary as needed.

Thanks!
-Derek

p.s. Also crediting @benjifisher for the UI review and feedback, and myself for all the work on the summary, etc.

You can treat the opinion in #23 as a recommendation. The usability team will not block this issue if you choose to use radios.

Is "default revision" used elsewhere in the UI? On /node/2/translations I see it described as "English (Original language)".

Personally, I think it is clearer to tie the option to the action performed, not the result of that action. Unfortunately, that makes it a little longer:

Initial moderation state when adding a new translation:
(*) Default moderation state.
( ) Current moderation state of the original language.

I have a feeling that there is room for improvement.

Here’s a proof-of-concept patch to get us started on the new setting. Leaving needs work because:

1. Still needs tests. Lots of ‘em. ;)
2. Probably needs an upgrade path to set this new workflow config for existing sites.
3. There’s 1 weird case where something that normally works fine is failing for the first page load after a cache rebuild. That seems wonky and wrong. Not sure what’s going on. Left a @todo comment in the code about it, but we should resolve that before commit.

But, this at least gets us a (mostly) working setting on the workflow edit form that is honored by the appropriate code when creating a new translation. Reviews / refinements / improvements most welcome!

Meanwhile, here are some screenshots to show it in action:

/admin/config/workflow/workflows/manage/editorial:

/node/1/translations (when only the original language exists, no translations yet):

/es/node/1/translations/add/en/es (either without the patch, or with the setting configured as "Current moderation state of the original language”):

/es/node/1/translations/add/en/es with the setting configured as "Default moderation state”:

Thanks, bot. ;) All those fails are from the same problem:

Exception: Warning: Undefined array key "translation_default_moderation_state_behavior"

Easy fixes attached. The existing content_moderation Kernel tests now all pass locally, so I hope the bot is happier this time around. ;)

Also adding test coverage for the setting. core/modules/content_moderation/tests/src/Kernel/DefaultRevisionStateTest.php seemed like the easiest/best place to do so, since it could reuse most of what that test was already doing, and this setting is related to the default revision state. If we want to move this into a separate test class, we certainly can.

I just added an Open questions section to the summary. Copying here as a snapshot and for folks following along via email. ;)

1. Which UI is best for the setting?
2. Since all the code handles no value for this setting as if you want the current behavior, do we need an upgrade path and all that complication?
3. Do we want to add this setting to the default workflows shipped with core?
   1. core/profiles/standard/config/optional/workflows.workflow.editorial.yml?
   2. If so, should we set it to 'default' or 'current'? Seems like new sites should get 'default' behavior by default.
   3. Also demo_umami?
4. Does this issue need a CR?
5. Do we need more test coverage?
   1. Do we need to test the UI for this setting when configuring a workflow?
   2. Other?

Thanks!
-Derek

Adding another open question:

6. Should the #default_value for these radios use 'default' for new workflows? It's now defaulting to 'current' if missing from the config for BC on existing sites. But presumably we could notice when creating a new workflow and use a different #default_value to be the more intuitive 'default'.

This should fix the remaining functional tests. 🤞

Well drat, I found an edge case here that doesn't work as expected. 😢

If there's no field widget for moderation_state visible on the entity form, when you create a new translation, we still end up with the current moderation state of the original language, not the 'default' moderation state.

I'm not yet sure the right layer of the system to fix this, but at least here's a failing test about it. 😉 I also wasn't sure how to prove this with a Kernel test, so for now it's extending Functional/ModerationLocaleTest.

I believe this is due to Drupal\content_moderation\EntityOperations::updateOrCreateFromEntity() doing this:

    // Create the ContentModerationState entity for the inserted entity.
    $moderation_state = $entity->moderation_state->value;
    /** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
    if (!$moderation_state) {
      $moderation_state = $workflow->getTypePlugin()->getInitialState($entity)->id();
    }
    $content_moderation_state->set('content_entity_revision_id', $entity_revision_id);
    $content_moderation_state->set('moderation_state', $moderation_state);

If there's no widget for moderation state on the translation form, $entity->moderation_state->value returns 'published' here. Is this a reasonable spot to check if it's a new translation and enforce this workflow setting? Or is there a better place to define the default value of a moderation state for new translations if it's not being handled by the form itself?

The Kernel test proves that doing all this programmatically via the Entity API works fine, and the setting is respected. But once we're dealing with the forms via the UI, if the widget isn't around to handle this for us, we're still not getting the right initial state.

Can we just directly hack EntityOperations::updateOrCreateFromEntity() about it?

Added that last point as another open question:

7. Can we directly hack EntityOperations::updateOrCreateFromEntity() about this new setting to handle the case where a translation form doesn't include a widget for moderation_state to get this right?

Whoops, forgot to check phpcs. This is now clear for all of core/modules/content_moderation...

Updating the open question #7 to be more clear:

7. Where is the right layer of the entity / translation / content_moderation systems to specify the default moderation state of a new translation? See comment #33. We don't want to have to rely on field widgets to enforce this setting. It'd be better if we directly changed the moderation_state when populating a new translation so that field widgets don't need to support this setting, and so it works if there is no widget for moderation_state. Should we?
   1. Use hook_entity_translation_create()
   2. Directly hack EntityOperations::updateOrCreateFromEntity()? This seems problematic since we'll have a hard time knowing if the entity has a value from a widget or if its own override logic should kick-in.
   3. Other?

Why not give the author role access to edit permission from "needs review"?

@lokapujya: For all sorts of reasons:

1. Some sites might require that once something moves to 'In review' that the original author can no longer edit it. They could be messing with what editors are looking at out from under them, there could be race conditions between when an editor looks at it and decides it's okay and the author could sneak in some unwanted changes, etc, etc.
2. Just because the original language version of a post is live doesn't mean that a brand new translation should also be live. Some sites might want to do an editorial review of the translation, too.
3. ...

The most logical thing would be for a new translation to work as if it was a brand new node and start with the default moderation state. That's what this issue allows. However, some sites might be relying on the (IMHO weird) existing behavior, so we're trying to support everyone's possible workflow configurations and needs.

Thanks,
-Derek

I've been trying to get feedback on the open questions, especially #7. However, in the absence of more expert opinions, I'm going with hook_entity_translation_create() for now since it seems to be the most elegant / simple solution. The function test I added now passes. All the tests pass if we revert the changes to core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php, but I think the UI is nicer if we keep those widget changes in the patch. Then, if this setting is enabled, the widget makes things look a little more simple and clear on a new translation. But if we wanted to leave those out, we now can.

Since we've got passing tests and a functionally complete patch, back to NR (for now, although it probably needs work, pending the answers to the open questions).

Thanks,
-Derek

@dww / Derek, thank-you for all of your work on this, I will test your patch shortly. Our use case is that we are using webform content creator to create nodes automatically from a webform, and in code we create the translation and found that it would never take on the default moderation state.

I wish I could help with your open questions but my understanding of this part of Drupal is insufficient. Thank-you again for all your hard work on this really difficult problem.

@dww - I have to update the system to D9 where I had to use this approach next week, so I will get this tested at least in our environment. I see a lot has changed since I was last here. Thanks for getting a patch out there, as well as the test setup, which I am sure was the much heavier lift.

I have been thinking that one of the root issues of this problem is how content_translation works. (as you pointed out) Other content management systems I have used usually have 3 options for translating a field.

• Not translatable
• Translatable, copy contents from source
• Translatable, don't copy contents

Option 3 is missing in content_translation and is why there is the hook_entity_tranlation_create "hack". I tried looking a while back if this had been logged as an issue, or if there is a maintainer for content_translation, but I did not come up with anything. If content_translation had an option to do this, then this issue would probably have been less circuitous.

However, at this point, changing translation field options is probably a bit much -- we might as well add the workflow state to the node_field_data table and actually make workflow a first class citizen. (Much like content_translation is)

I was asked to give my input on the 7 open questions.

Here is my 2c

1. Which UI is best for the setting?I personally think the radio option is the more descriptive of the two, but I'd defer to the UX team
2. Since all the code handles no value for this setting as if you want the current behavior, do we need an upgrade path and all that complication?We'd need an update path to set the default value to the current behaviour
3. Do we want to add this setting to the default workflows shipped with core?I'd vote to leave it as is (current)
4. Does this issue need a CR?Yes, and I'd argue its a feature request rather than a bug, which is why I think it needs a CR. Not re-opening that can of worms though. Either way there's a UI change which warrants a CR.
5. Do we need more test coverage?Yes, at least a kernel test covering the scenario in the issue summary
6. Should the #default_value for these radios use 'default' for new workflows?I'd leave it as 'current'
7. Where is the right layer of the entity / translation / content_moderation systems to specify the default moderation state Translations are not my strong spot. But I agree that doing it in the form layer is wrong as it would second-class JSON:API should we ever get translation support there. Probably best to ask someone who knows more about the l10n systems.

Thanks for answering the questions, @larowlan! Added a CR, updating the summary, tagging for NW, and a few follow-up questions:

1. UI: Agreed. Let's see if we get more feedback, but the UX team already said at #26 "You can treat the opinion in #23 as a recommendation. The usability team will not block this issue if you choose to use radios."
2. In that case, needs work for an upgrade path.
3. Duly noted. We'll need to add the setting value so new sites get valid config, right?. I'll leave the question open in case Umami wants a different default or something.
4. Draft CR: https://www.drupal.org/node/3240957
5. Tagging for more tests. Do we need to test the UI for this setting when configuring a workflow?
6. Duly noted. I'll leave it alone for now, but leave the question open.
7. Duly noted. 😉 I'll keep trying to get Berdir or someone else similarly knowledgable (does anyone exist?) to shine light on this. 🙏

Do we need to test the UI for this setting when configuring a workflow?

Technically yes

Usability review

We discussed this issue at #3240281: Drupal Usability Meeting 2021-10-08. That issue will have link to a recording of the meeting.

This meeting had more participants than the one on 2021-07-16 (see Comment #23). I am not sure whether the difference is having a different group or if the points in #25 convinced us, but this time we agree that radio buttons are clearer. There was pretty strong consensus on that.

I see that the screenshot in the issue summary (and in #27) uses the wording I suggested in #26. As I said then, there is room for improvement. The tricky part is how to describe the default revision. In #26, I suggested "original language", based on the existing text on the translations tab such as /node/2/translations. Looking at it again, I prefer "original content" here, and the usability group agreed that is an improvement:

Initial moderation state when adding a new translation:
(*) Default moderation state
( ) Current moderation state of the original content

But maybe you can come up with something even better.

The other thing I notice, looking at it again, is that in #26 I included ending punctuation. The two options are not complete sentences, so you were right to remove that.

Still Needs upgrade path + Needs tests, but here are a few fixes per #42 and #45, and cleanup of the open questions in the summary. More coming later today...

Okay, after a fun Slack session with @larowlan, I sorted out Kernel test coverage for the scenario in the original bug report. So that's now working exactly as expected.

Also fleshed out the Functional test coverage we already have to make sure the new knob disappears from the workflow edit form when you uninstall content_translation.

So I'm removing the generic "Needs tests" tag and adding "Needs upgrade path tests" since those are the only tests we're still missing. More cleanup of open questions and remaining tasks.

The only open question remaining is:

7. Where is the right layer of the entity / translation / content_moderation systems to specify the default moderation state of a new translation? See comment #33. We don't want to have to rely on field widgets to enforce this setting. It'd be better if we directly changed the moderation_state when populating a new translation so that field widgets don't need to support this setting, and so it works if there is no widget for moderation_state. Should we?
   1. Use hook_entity_translation_create()
   2. Directly hack EntityOperations::updateOrCreateFromEntity()? This seems problematic since we'll have a hard time knowing if the entity has a value from a widget or if its own override logic should kick-in.
   3. Other?

The only remaining task is: "Add a DB update for existing sites"

Home stretch now 🤞

Thanks,
-Derek

p.s. Assigning to myself since I'm working on the upgrade path now...

The post_update itself was easy. The upgrade path test is more yucky since the existing system/tests/fixtures/update/drupal-9.0.0.filled.standard.php.gz doesn't have content_moderation installed. https://www.drupal.org/docs/drupal-apis/update-api/writing-automated-upd... doesn't really explain how to generate the setup file to install whole new modules. 8.9.x's copy of content_moderation has some of these setup files, but I don't think we can re-use any of that.

#49 is a random fail in core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderDisableInteractionsTest.php:185. @see #3208791: [random test failure] Random fail in LayoutBuilderDisableInteractionsTest.

Anyway, here's most of an update path test. I'm kinda fumbling around in the dark here. https://www.drupal.org/docs/drupal-apis/update-api/writing-automated-upd... doesn't explain how to generate a test fixture setup file that enables a different module and its default config from a given prior state. I'm looking around for existing examples, and there’s not much to draw on from 9.3.x branch. core/modules/layout_builder/tests/fixtures/update/layout-builder.php has some of the necessary magic. I’m making some progress, but hitting a wall with this:

There was 1 failure:

1) Drupal\Tests\content_moderation\Functional\Update\TranslationDefaultModerationStateBehaviorUpdateTest::testUpdateTranslationDefaultModerationStateBehavior
Content moderation state: The <em class="placeholder">Content moderation state</em> entity type needs to be installed.

/.../core/tests/Drupal/Tests/UpdatePathTestTrait.php:150
/.../core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBase.php:270
/.../core/modules/content_moderation/tests/src/Functional/Update/TranslationDefaultModerationStateBehaviorUpdateTest.php:35
/.../vendor/phpunit/phpunit/src/Framework/TestResult.php:703

Not sure how to craft the setup script so that entity type gets installed properly and everything works as expected.

Is there any easier way to do this?

Any pointers / insights would be most appreciated!

Thanks,
-Derek

Oh yeah, the patch. 😉

In Slack, @larowlan suggested using core/scripts/dump-database-d8-mysql.php for this. On its own, that'd create a huge new fixture, which seems like a big waste. However, I figured out a way to use that to solve my troubles:

1. Started from clean install of 9.0.0 with 'standard' profile.
2. Ran core/scripts/dump-database-d8-mysql.php to generate a "bare" 9.0.0 dump.
3. Installed content_moderation + workflows.
4. Ran dump-database-d8-mysql.php again to get the new version.
5. Diff'ed the two dumps to figure out what we're missing. Wow, there's a lot that gets injected into key_value when you define new entity types! 😉TIL. 🤓
6. Copied the relevant bits into content_moderation.php. In addition to the entity system state, the previous version didn't even define any of the DB tables, etc. But it's all there now.
7. Test is now passing! 🎉

I preemptively ran phpcbf on the results to convert to short array syntax so that the testbot doesn't complain about CS violations.

Realized the previous patch left out another section of the diff between the two DB snapshots. I was juggling too many diffs and missed some. Since setting up this fixture is such a pain, I figured I should do a thorough / complete job now in the hope that it'll be helpful to someone else down the road. Since I was totally setup for this mess, I wanted to really finish, even though the test passes with #53 alone.

p.s. Added a section to https://www.drupal.org/docs/drupal-apis/update-api/writing-automated-upd... about how to do this.

Whoops. Turned off code style, but forgot to disable cspell, too. 🤦‍♂️ Let's try this...

I think option 7 has been resolved, the hook is the API core makes available for it, doing it in a widget is wrong. We can probably mark that as resolved

Here's some observations/questions

1. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
   @@ -121,10 +121,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // @todo Figure out what's happening here: we shouldn't need this code.
   

   should we be resolving this here?

2. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
   @@ -121,10 +121,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // For BC, if the configuration doesn't define it, use the legacy behavior.
   

   now there's an update hook, this shouldn't happen now right?

3. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -308,7 +308,13 @@ public function getInitialState($entity = NULL) {
   +    $is_new = $entity->isNew() ||
   +            ($entity->isNewTranslation()
   

   this is the third instance of this logic, should it live somewhere central?

4. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -308,7 +308,13 @@ public function getInitialState($entity = NULL) {
   +             && isset($this->configuration['translation_default_moderation_state_behavior'])
   

   this should always be set now right? we could set it in the constructor to default if not and avoid this check?

Thanks for the review!

Closing open question 7. Per Slack chat, moved the logic that implements hook_entity_translation_create() into \Drupal\content_moderation\EntityOperations::entityTranslationCreate() and stuck it right next to the existing entityTranslationDelete() method. Moved content_moderation_entity_translation_create() up in content_moderation.module to be next to content_moderation_entity_translation_delete(), too.

Now for the review points:

1. Oh right, completely forgot that @todo was in there. Thanks for reminding me!

   I did some more local digging and discovered that on the first page load after a cache rebuild, inside ModerationStateWidget::formElement(), this is returning the wrong bundle ID: $this->fieldDefinition->getTargetBundle();. If I inspect the value, on the first load, I get "page" (wrong), which is why the lookup for the workflow fails and we access $configuration even though it's empty. Next page load, it's back to "article" (correct), and remains that way.

   This is all true in the clean 9.3.x branch, too. Totally unrelated to this issue. In Slack, @Sam152 pointed us to #3077876: content_moderation_entity_bundle_field_info overrides the target bundle setting on the field definition for all bundles, which sounds like the underlying bug. No followup needed.

   Regardless, I removed the @todo and simplified the code to use $entity->bundle() instead. That always works (and is more clear / direct to express what this code is trying to do).

2. 👍Good point, removed.
3. Rule of Three, indeed. Although I'm not totally sure where to put this code so it could be shared, since the 3 callers don't all have the same context to work with. 🤔
4. I thought so, but when I removed it, I started getting a notice in the new Kernel tests. But we can define the default in WorkflowType/ContentModeration::defaultConfiguration() and it works again. That seems cleaner/better than the isset().

So, potentially NW for the Rule of Three refactoring, but uploading a new patch for now, in case we're cool leaving this as-is.

Thanks again!
-Derek

Hah, whoops. #58.4 breaks the upgrade path test, since that default setting now shows up in the "before" state, even though the setting isn't present in the DB dump fixture.

Reverted using ContentModeration::defaultConfiguration(). Instead, to make the Kernel tests happy, I added a default for translation_default_moderation_state_behavior in ContentModerationTestTrait::createEditorialWorkflow().

Is that legit?

Or is #58.4 more correct, and we shouldn't worry about an upgrade path at all? 😢

Thoughts?

Thanks,
-Derek

In Slack, we discussed this more. The last few patches are Doing It Wrong(tm). 😉 I’ve got a new patch to upload here that I worked on while offline sitting on a plane. just need to get my laptop online sometime this evening to upload it. Stay tuned....

Per @Sam152 in Slack, #2543150: Document consequences of contrib changing config schema without core's API supporting config version tracking says we need both an update path and to support things if the value isn't there. So I restored the isset() and friends, removed the default config, and got everything happy again.

I've gone over this again.

And yes, Sam is right about needing to handle when the config isn't set - we do that for views because e.g. they could be broken in a profile.

However, in this case we can set a default value in the defaultConfiguration method so I don't think it applies. But I'm not going to split hairs on it.

This looks ready to go.

1. +++ b/core/modules/content_moderation/src/Form/ContentModerationConfigureForm.php
   @@ -153,6 +175,7 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +    $configuration['translation_default_moderation_state_behavior'] = $form_state->getValue(['workflow_settings', 'translation_default_moderation_state_behavior']);
   

   I think if content_translation is not installed this will change the value from 'current' which has been set by content_moderation_post_update_set_translation_default_moderation_state_behavior to a NULL.

2. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
   @@ -121,10 +121,23 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // If the configuration doesn't define it, use the legacy behavior. We
   +    // cannot rely on the update path having been run, so we must not generate a
   +    // notice by trying to access an undefined property.
   +    if (empty($configuration['translation_default_moderation_state_behavior'])) {
   +      $configuration['translation_default_moderation_state_behavior'] = 'current';
   +    }
   

   This also saves us when the configuration has been saved again when the content translation module is not installed.

   I think the behaviour here is very awkward and needs thinking about. The best thing to do will be to set the current value as a hidden value on the form when content translation is not installed.

3. I wish this was being added as a third party setting and a form alter from content translation rather than adding switches into content moderation. From the POV of config. That said we probably don't have a way to delegate getting the initial state to a third party so perhaps this is the best we can do.

@alexpott Thanks for the careful review - good catch. Working on fixing 64.1 + 2 now.

Re: 64.3 - yeah, I don't know how this feature could entirely live in content_translation without much deeper API changes. core/modules/content_moderation/src/ModerationInformation.php gives us no means to try to influence the outcome of getOriginalState(). Adding an alter hook in there seems pretty strange. So yeah, I think we have to leave this basically as-is...

Fixes 64.1 + 2, and adds a little test coverage of it. The test-only is just the new test coverage, without the fix, and a drupalci.yml change to only run the failure test. Should die like so:

There was 1 failure:

1) Drupal\Tests\content_moderation\Functional\ModerationLocaleTest::testTranslateModeratedContent
Failed asserting that null matches expected 'default'.

I don't know why I mess up drupalci.yml changes so frequently, trying to save bot cycles. 🤦‍♂️ Anyway, I don't want to fiddle with it now, since it'll just confuse the bot if I upload the test-only here. But the output above is from a local run. If you don't trust me, you can apply these two:

• https://www.drupal.org/files/issues/2021-10-21/3150294-62.patch
• https://www.drupal.org/files/issues/2021-10-25/3150294.62_66-test-only.i...

And then run it locally yourself:

./vendor/bin/phpunit -c core/phpunit.xml core/modules/content_moderation/tests/src/Functional/ModerationLocaleTest.php --filter testTranslateModeratedContent

+++ b/core/drupalci.yml
@@ -13,32 +13,9 @@ build:
+        testgroups: '--class="Drupal\Tests\content_moderation\Functional\ModerationLocaleTest"'

p.s. This is why: run-test.sh doesn't like the = here. This should be:

testgroups: '--class "Drupal\Tests\content_moderation\Functional\ModerationLocaleTest"'

Anyway, still don't think it's worth a whole new pair of patches here, but happy to upload them if we need. I believe this is RTBC, but I'm obviously not qualified to change the status.

1. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
   @@ -121,10 +121,23 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // If the configuration doesn't define it, use the legacy behavior. We
   +    // cannot rely on the update path having been run, so we must not generate a
   +    // notice by trying to access an undefined property.
   +    if (empty($configuration['translation_default_moderation_state_behavior'])) {
   +      $configuration['translation_default_moderation_state_behavior'] = 'current';
   +    }
   

   This is handled as an isset() call in the ContentModeration plugin, why do it that way there and this way here? I personally find the logic easier to follow in the plugin version. I think this may be what @alexpott meant by awkward behaviour?

2. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
   @@ -121,10 +121,23 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $is_new = $entity->isNew() || ($entity->isNewTranslation() && $configuration['translation_default_moderation_state_behavior'] === 'default');
   

   Nit(?): $entity->isNew() is self explanatory, this variable name is not completely so. As far as I can tell, the workflow is the new thing here, could we call this variable $new_workflow or similar (to improve DX)?

3. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -308,7 +308,13 @@ public function getInitialState($entity = NULL) {
   +    $is_new = $entity->isNew() ||
   

   Same as #2.

Thanks for the review, @darvanen!

#69.1: Ahh, good point. That's a hold-over from the @todo weirdness I removed in #58.1. Now that that's actually working as expected, the code doesn't need to be doing it the way it was, and the isset() is sufficient. Fixed here.

Points 2 and 3 I'm a little reluctant to change. It's not a "workflow" that's new here. It's either that the entity itself is brand new, or it's a new translation of the entity, and the workflow is configured to treat new translations as if they're a new entity and start off in the default workflow state. I know $is_new isn't the most self-documenting variable name, but $new_workflow would definitely be misleading. Maybe $treat_as_new or something, but that seems a little clumsy, too. Given that neither @larowlan nor @alexpott flagged this part, I'm inclined to leave it alone.

The "better" solution would be to consolidate these checks per #57.3. Per later discussion, there's no particularly good place to share it, since each of the 3 is at a different layer of this system (field widget plugin, workflow type plugin, ModerationInfo service). But I just looked again, and realized that both the field widget and workflow type plugins have an injected copy of the ModerationInfo service. So if we wanted to, we could add a new public method on that service, something like ModerationInfo::treatAsNew(EntityInterface $entity) or something. But I'm not sure a new public API for this is worth adding. I'll await confirmation from a core committer on that before I do it.

Meanwhile, since it's a new patch, here's a (hopefully) proper fail patch for the new test in 66. 🤞

Yay, well that's at least progress. 😉

https://www.drupal.org/pift-ci-job/2221701

There was 1 failure:

1) Drupal\Tests\content_moderation\Functional\ModerationLocaleTest::testTranslateModeratedContent
Failed asserting that null matches expected 'default'.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/core/modules/content_moderation/tests/src/Functional/ModerationLocaleTest.php:384
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:722

The failed assertion is here:

    // Ensure we didn't clobber the setting value without content_translation.
    // Reload the workflow so we have the latest values.
    $this->workflow = Workflow::load($this->workflow->id());
    $configuration = $this->workflow->getTypePlugin()->getConfiguration();
    $this->assertEquals('default', $configuration['translation_default_moderation_state_behavior']);

So yay, the test fails if you don't have the fix for #64 1+2.

Progress is awesome :D

Fair enough re the variable naming, happy to let that go as it seems to come down to a personal interpretation.

I have just one request before I turn this green?:

+++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
@@ -121,10 +121,17 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    $is_new = $entity->isNew() || ($entity->isNewTranslation() && isset($configuration['translation_default_moderation_state_behavior']) && $configuration['translation_default_moderation_state_behavior'] === 'default');

Can we spread this over 4 lines like in the plugin? I think it's technically allowed to be that long (216 characters) as it's in an if statement but it doesn't really set a great precedent for readability in my opinion.

Confirmed with @larowlan via Slack that the proposal at the end of #70 for adding something like ModerationInfo::treatAsNew(EntityInterface $entity): bool would be unwelcome API surface. We've hit the rule of 3, but it's for a relatively small variable initialization, so it's not too much of a burden to duplicate the code in the 3 places we need it. Still happy to add it if needed, but it does seem like premature API-ification to both of us.

Therefore, it was worth fixing the readability of one of the copies of it per #73. Done here.

Anything else?

Thanks!
-Derek

We've hit the rule of 3, but it's for a relatively small variable initialization, so it's not too much of a burden to duplicate the code in the 3 places we need it. Still happy to add it if needed, but it does seem like premature API-ification to both of us.

Makes sense.

Anything else?

Not from me!

I don't expect the test to fail and I have to leave early today so I'm gonna switch this now, if there is a failure somehow the bot will handle it :)

Thanks! Very happy to see this RTBC again. Saving credit for @darvanen for the useful reviews.

Adding 'content moderation multilingual bug' as an issue tag I found when looking at related issues.

Tested patch against 9.1 and it works perfectly. Thank you @dww for the amazing effort on this one and all the others that contributed!

I just came across this patch and applied it to 9.2.6 without errors! Works perfectly and does exactly what we need. Thanks for all your efforts

re: #77 and #78: Thanks! Glad it's working well for you both.

Weird that the bot determined that the patch no longer applies but left the status as RTBC.

Anyway, this needed a trivial re-roll after #3222251: [November 8, 2021] Replace all isset constructs with the null coalescing operator landed. No interdiff, but here's a raw diff of the 2 patch files to see the context changed. Leaving RTBC.

FYI: bot weirdness is here: #3047797: Patch failing to apply should reset RTBC status to Needs Work

I tried to apply patch #79 for the Drupal 9.2.7 version but the patch was failing.

Re-rolled the patch for the Drupal 9.2.7 version.

Re-uploading #79 which is still RTBC.

Back to RTBC per #84 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

This seems really close. I love how none of the existing test cases seemed to require modification to get this to pass, adds a lot of confidence in the BC layer.

Not going to change the status, because a committer might disagree with these observations:

1. +++ b/core/modules/content_moderation/src/EntityOperations.php
   @@ -248,6 +248,32 @@ public function entityRevisionDelete(EntityInterface $entity) {
   +      if ($translation instanceof EntityPublishedInterface) {
   +        $translation->setUnpublished();
   +      }
   

   I'm not sure if this has been explained or discussed in this issue, but feels like this should have an explanation in a code comment.

   I can't make sense of why this is required, if the implementation of `getInitialState` has already been modified to be aware of the configuration flag?

2. +++ b/core/modules/content_moderation/src/Plugin/Field/FieldWidget/ModerationStateWidget.php
   @@ -121,10 +121,20 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // How to handle the default moderation state for a new translation is
   +    // controlled by a setting. In various places in the rest of this method, we
   +    // need to know if something is "new". Compute it once so we can reuse it.
   ...
   +    $is_new = $entity->isNew() ||
   +            ($entity->isNewTranslation()
   +             && isset($configuration['translation_default_moderation_state_behavior'])
   +             && $configuration['translation_default_moderation_state_behavior'] === 'default');
   

   Not a huge fan of calling this variable "is_new", when it represents "is new or is a new translation and the new translation behavior has been assigned to default". Couple of additional booleans here would probably clean this up a bit.

   This also applies to where this has been copied to. (Also this is more a preference thing, but I would just bite the bullet and put some of this stuff behind methods, being used in two places seems like enough of a signal to create a function to me)

3. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -308,7 +308,13 @@ public function getInitialState($entity = NULL) {
   +             && isset($this->configuration['translation_default_moderation_state_behavior'])
   +             && $this->configuration['translation_default_moderation_state_behavior'] === 'default');
   

   Wonder if we can just put this tricky isset/default comparison behind a isTranslatioNBehaviorDefault/isTranslationBehaviorCurrent or perhaps `getTranslationBehaviour(): string` on the plugin class.

   Also feels like current/default are candidates for constants (or an enum, if we have that option).

4. +++ b/core/modules/content_moderation/tests/src/Kernel/DefaultRevisionStateTest.php
   @@ -113,6 +126,187 @@ public function testMultilingual() {
   +    // Make sure the default state for new nodes is 'draft'.
   ...
   +    // Change the workflow setting to use 'default', and set that to 'draft'.
   +    $configuration['translation_default_moderation_state_behavior'] = 'default';
   +    $this->workflow->getTypePlugin()->setConfiguration($configuration);
   +    $this->workflow->save();
   

   I'd love to see a test case where "default_moderation_state" was set to both published default and unpublished non default states that were neither "published" or "draft".

Patch 79/84 needs a reroll.

While you're at it, you might want to take #87 in to consideration.

Thanks for the reviews and updates! I'll do a reroll, then address #87. Stay tuned...

Here's a straight re-roll for 9.4.x. Had a fairly trivial conflict to resolve. Interdiff is confused, but here's the raw diff. Leaving assigned to myself to work on 87.

Re #87: Thanks for the careful review! Much appreciated.

1. Great question! I'm honestly not sure. 🤔 Git blame isn't helping jog my memory as to why this would be needed. I ripped it out locally and all the Kernel tests still pass. I'm going to remove it as part of the patch here to see what the bot thinks with a full run. That's 3150294-91.main.patch.
2. I hear you. We've been here before. See #57.3, #69.2, #70, #74, etc. I guess you're voting the other way. 😉 I know you're the subsystem maintainer, but I'm deferring to @larowlan's take as a committer until they change their mind and/or other committers weigh in that they'd rather see ModerationInfo::treatAsNew(EntityInterface $entity): bool or equivalent.
3. I'm not sure this is worth a separate function. If we're going to consolidate, this check will only appear once. Perhaps in the refactoring we can use ?? nicely to make it more simple and clear. Anyway, not changing this until a committer says so.
4. Good point! I got some clarity via Slack on exactly what this was about. Both 'draft' and 'published' are hardcoded in various spots. I changed some of the new test plumbing to stop using draft and published by adding a new workflow with both those (since they're required) and their alternates 'sketch' and 'live'.

   However, when I did this, the test was failing like so:

   There was 1 failure:
   
   1) Drupal\Tests\content_moderation\Kernel\DefaultRevisionStateTest::testTranslationDefaultModerationState
   Failed asserting that two strings are equal.
   --- Expected
   +++ Actual
   @@ @@
   -'live'
   +'published'
   

   I went digging, and discovered something right under our noses (in the patch context). The current behavior (unchanged by this patch) when something is supposed to use the "current" / legacy behavior is not what all of us assumed it was: "inherit the current state of the original language". It's actually this:

       if ($entity instanceof EntityPublishedInterface && !$is_new) {
         return $this->getState($entity->isPublished() ? 'published' : 'draft');
       }
   

   🤯 Whoops. So the BC layer is implementing the wrong UI to describe the legacy / existing behavior. It's not "Current moderation state of the original content", it's something like "Set to 'published' if the original language is currently in any published state, otherwise 'draft'." That's of course horrible UI text. 😉 We'll have to figure it out.

Not sure where to go from here. Some possible approaches:

A) Rename the UI for the new "current" setting to more accurately explain the current / legacy behavior. If we just keep 2 options, I'd strongly consider naming this one "legacy" not "current".

B) Provide 3 options in the new workflow plugin settings for the behavior:

1. Default (added here)
2. Current (another new behavior we'd have to implement properly, what the existing patch explains in the UI setting)
3. Legacy (the existing hard-coded behavior of only 'draft' vs. 'published')

C) Other? Not sure...

I don't think we can plow forward with the current disagreement between the UI text and the actual behavior. I do think we're going to have to keep supporting the current behavior, so we're going to have to figure out what to call it and how to explain how it works to site builders configuring workflows.

Thoughts?

Thanks/sorry,
-Derek

The analysis in #91 sounds pretty spot on to me. I have no idea off the top of my head if/why/how that code path is entered for new translations, but I do agree it probably needs to stick. "Legacy" does seem to describe it, given it's not logical or intended.

Whoops, typo in the new test comments. sh ./core/scripts/dev/commit-code-check.sh --branch 9.4.x is now passing locally. Let's try that again...

Sam and I continued to discuss in Slack. We agreed that #91.B is too much scope creep. So I opened #3257186: [PP-1] Add a 'Current' option to the workflow translation_default_moderation_state_behavior setting to track adding 'Current' as an option to this behavior setting once this issue lands.

Meanwhile, here's a preliminary stab at changing the new setting from 'current' to 'legacy'. Here's my very sad initial UI text:

Is there any way to get the "Default moderation state" label to include what that default is set to?

Are Published and Draft really the only possible options for the legacy behaviour?

Re: #96 question 1:

1. The current default is the immediately prior setting on the page:
   
   Although the wording of the new setting is changing, its placement in the form is not, so the context you need is right there.
2. You could change that setting while you're on the page. So to be useful, this thing would need to be dynamically updated. However, #states doesn't support something like that so it'd require custom JS.
3. I don't know of anywhere else in the core UI where we try to do something like this. 🤔 At least not when we're talking about two adjacent elements on the same form. If you can think of an example, please provide it. 😉

Re: question 2: Yes, the existing code in Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::getInitialState() does this:

    if ($entity instanceof EntityPublishedInterface && !$entity->isNew()) {
      return $this->getState($entity->isPublished() ? 'published' : 'draft');
    }

Re: #92:

Git blame points here:

commit b6a29fd2f452e576be691f57214924e42ee80f11
Author: Alex Pott <alex.a.pott@googlemail.com>
Date:   Fri Sep 21 12:58:56 2018 +0100

    Issue #2856967 by Sam152, yongt9412, alexpott, Berdir, larowlan, timmillwood, nlisgo: Allow admins to select a default starting moderation state

So #2856967: Allow admins to select a default entity moderation state was the last to touch this code. However, digging deeper into the Git archeology, that was just refactoring existing logic. This legacy behavior has been touched by all these commits:

• commit d4165ff10 #2900046: $workflow param is no longer required in getInitialState
• commit a16b9c74c #2893778: Remove deprecated Workflows methods
• commit 69555bcd #2884715: Initial Content Moderation transitions are not correct

But it was actually introduced by commit 469003db for #2817835: When enabling moderation apply a relative state. I skimmed that issue, but didn't see any clear / direct discussion of this behavior as being intentional and desirable.

Not sure that was all that helpful of a history dive, but there you have it. 😉 Maybe that'll help jog @Sam152's memory, but probably not.

Anyway, I think the next steps are to bikeshed the new UI text again. Maybe the UX team would be willing to re-review this. But if anyone has suggestions for improvement over what's in #95, please post them here.

Thanks!
-Derek

Thanks for the very detailed answer!

I was just looking for potential logical issues with the wording and those have been blown out of the water. I think the wording is fine as is.

Re: #98: Thanks! But I'm sure we can do better. 😉

Updating the summary:

• Removed proposed resolutions we ruled out.
• Updated the "After" screenshot with the current UI from #95.
• Updated open questions / remaining tasks.

I hope this is now clear enough for a UX team re-review.

Officially requested UX team re-review at #3257468-2: Drupal Usability Meeting 2022-01-14. If anyone is available "tomorrow" "morning" (depending on your TZ) to attend the UX meeting and present this, that'd be great. Details in the linked agenda issue. If not, hopefully they now have enough to go on.

We discussed the UX wording for the option at #3261138: Drupal Usability Meeting 2022-02-04. That issue will have a link to the recording.

Proposed wording when content translation & content moderation are enabled

Initial moderation state when translating existing content
1) Default moderation state
2) Published if existing content is published, otherwise Draft

I attended the Usability meeting today.

I am setting the IS to NW for the change recommended in #101.

In #101, we suggest using the phrase "existing content" in the label and then in the second option. We think this will help clarify the second option. If you do not like "existing content", then a similar phrase should work just as well.

Our suggested text removes "Legacy behavior: either". We feel this is not actionable information. That it, it does not help the site builder decide which option to choose. Even after removing this bit, I think we managed to make the text shorter and more explicit.

Some people felt that, even though the new option comes directly below "Default moderation state", the first option would be clearer if you added the currently selected value, like "Default moderation state (Draft)". Consider that an optional enhancement.

Some felt that "new content" in the existing help text is not enough to clarify the distinction between the two options. Perhaps, when Content Translation is enabled, we could add a line to that help text. I guess something like, "Here, 'new content' does not include a new translation of existing content. For that, the next option applies." But we did not come up with a definite suggestion, so we can leave this as is for now. My personal opinion is to follow the usual rule that "less is more".

For the record: the participants at today's meeting were

benjifisher, g-brodiei, rkoller, worldlinemine, AaronMcHale, mariaisp, victoria-marina, shaal, tmaiochi

Also to note that for the second radio button, the terms "Published" and "Draft" are italicized to indicate that those would be substituted with the labels for the Draft and Published states.

Thanks everyone for the review and vastly improved UI text suggestions!

Implemented here. Also a fresh rebase to 9.4.x. Confirmed this applies cleanly to 10.0.x. New screenshot in the summary. Also closed out open questions and updated remaining tasks. RTBC is all that's left. 😉

#101 looks super clean and simple. Way better than #95. Thanks!

Re: #102: All the suggestions for improvement are great and make sense. Thanks for explaining the reasoning so that we can all learn and benefit from the thoughtful discussion.

Some people felt that, even though the new option comes directly below "Default moderation state", the first option would be clearer if you added the currently selected value, like "Default moderation state (Draft)". Consider that an optional enhancement.

#96 already proposed this, and I replied in #97 (first 3 points) why I think that shouldn't happen here, if at all. 😉 I believe it's scope creep for this bug fix, and belongs in a follow-up feature request. Not going to tag for "needs followup" but if someone is inspired to open it as a child issue, please do.

Also happy if anyone wants to open a follow-up to conditionally modify the existing help text for the Default moderation state setting when content_translation is enabled. I agree with less is more, and I don't think it's needed, but others might disagree. The new Initial moderation state when translating existing content setting only appears if content_translation is enabled at all, so hopefully it's clear enough as-is.

#103: Thanks for clarifying. That's what #95 was doing already, so I assumed y'all meant to keep that behavior.

Ready for a (final?) review and RTBC.

Thanks again, everyone!
-Derek

Wrong # for the files, sorry about that. ;) Fixing a few things in the summary.

This does add some complexity to a few areas of CM and translations have been one of the harder things to deal with, especially given the lack of active maintainers with deep understanding of the translation system.

However, I think given the number of times this headache has been reported, the number of followers the issue has, the diligence in preserving BC, good automated testing, pivoting and following up UX, the extremely detailed issue summary, the fact that it'll only appear for folks doing translations.. this is ready for commit and should hopefully make a lot of existing and new users quite happy.

Massive effort, especially dealing with a handful of extremely complicated subsystems, nice one @dww.

If committed, the CR might need a tweak, but we should definitely not block behind that.

Changing version to 10.0.x since this needs to be applied there first. Also, starting a test on 10.0.x

This update test needs to use the new fixture from #3263886: Copy drupal-9.3.0.bare.standard.php.gz and drupal-9.3.0.filled.standard.php.gz from the Drupal 10 branch, then it should pass on all the branches. I’ll work on it after breakfast. Stay tuned.

Work got busy after breakfast 😅, but here we are...

Given it's a one-line change that exactly meets the reason for putting this to NW and it's passing, this is now back to RTBC.

1. +++ b/core/modules/content_moderation/src/Form/ContentModerationConfigureForm.php
   @@ -136,14 +146,40 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
   +    // Either way, we want the same default value for this: the currently
   +    // configured value, or 'legacy' as a fallback to preserve legacy behavior
   +    // on sites that haven't updated their config.
   +    $form['workflow_settings']['translation_default_moderation_state_behavior']['#default_value'] = $workflow_type_configuration['translation_default_moderation_state_behavior'] ?? 'legacy';
   

   This is really odd. Firstly we should never be concerned with sites that haven't updated their config. Runtime code should assume that all updates have been run. We don't support running code with outstanding updates.

   Secondly, I think that this means that the default value for this is legacy - so if you start fresh and enable content moderation and then at some point in the future enable content translation by default you'll be in the legacy mode. I'm not sure that that makes sense.

2. +++ b/core/modules/content_moderation/tests/fixtures/update/content_moderation.php
   @@ -0,0 +1,689 @@
   + * After system/tests/fixtures/update/drupal-9.0.0.filled.standard.php.gz is
   + * loaded, this file will alter the database to the state of a fresh install of
   + * Drupal 9.0.0 with content_moderation and workflows modules enabled, including
   + * all of their default configuration.
   

   Looks like this might need updating.

This is really odd. Firstly we should never be concerned with sites that haven't updated their config. Runtime code should assume that all updates have been run. We don't support running code with outstanding updates.

I think this was in the same vein as when we add a hook views presave for views config changes.

i.e. there maybe configuration in an install profile that is invalid.

See #62 for where that came about

Yeah, I've gotten that feedback in the past, but it often gets questioned when implemented, so I've either misunderstood that feedback or it's not widely accepted as a problem.

I could however definitely conceive of a bunch of workflow configs floating around in various forms that aren't being affected by an updb process. For any site installed after this issue is committed, the updb won't impact them and importing those config files will leave those installations broken.

Re: #111:

1. I don't know what to say. 😉 I've implemented what @larowlan and @Sam152 asked for, and included links to the various places where what they're asking for was agreed / decided / etc. I truly don't see the harm in having this code be resilient in the face of the unexpected. If you want me to intentionally fragile-ify the code and make it so that things will just fatal error if folks don't update their config, so be it. But a. ugh, I wish everyone was on the same page about this in the first place and b. I don't understand what we gain by making this more fragile. I don't know that it would significantly simplify the code. It's already written and tested to be defensive.

2. Yeah, my heart truly sank when I started working on #109 and remembered what a massive effort it was to get this update test working in the first place, only to have #3261486: Remove core updates added prior to 9.3.0 and adjust test coverage remove the fixture it depended on. 😢 I was being a bit of a cowboy, and instead of doing it all over again, I just made the 1 line change and uploaded it to see what the bot said. 😅 To my great delight, it all just works. So the fixture I built to content_moderation-ify a standard install works just as well starting from 9.3.0 as it did from 9.0.0. 🎉 Phew! Therefore, here's a patch to update the comments accordingly. 😉

Back to NR. I don't want to start writing any code for 111.1 until there's agreement on what y'all want.

Thanks,
-Derek

Reviewed and tested the above patch with 10.0.x. Adding screenshots for the references

@Rinku Jacob 13: The summary already has accurate and updated screenshots. Removing credit for attaching duplicate screenshots.

If you want to be credited in an issue for attaching screenshots, I suggest:

1. Do it to an issue that doesn't already have screenshots.
2. Update the issue summary to embed the screenshots (usually in the User interface changes section) or add links to them.

If you do neither, you're not really helping move the issue forward with this activity, so core maintainers / committers are likely to remove your credit.

There's a "Needs screenshots" tag that you can work from if you need help finding issues for step #1. 😉 If you attach screenshots to one of those issues, and update the summary, you can then remove the tag. That's actually helpful to getting issues marked fixed, and worth granting credit for.

Thanks,
-Derek

Tested this out locally on 10.0.x (not sure if this will move to 10.1.x)
Applied cleanly
Followed the testing steps written in the description and verified it worked.

Side note that is one of the best issue summaries I've read yet. Super clear every section.

I see there are no remaining tasks so I'll mark it RTBC.

Thanks!

Can I move back to RTBC? Just updated 9.3 to 9.4 not a big change.

@smustgrave the latest patch doesn’t apply to my 9.4.7 via composer because of the change to the .gitignore file.
Once I removed that it applied fine. I might be wrong, but I don't think this module should be affecting that file?

it should not.

Here's the patch from #120 with the gitignore changes (as mentioned in #122) removed. It now applies properly to the latest stable Drupal as of this is writing.

we've been using patch 109 with D10.0.10 and previous Drupals for a while now

has tests and passes tests. What is left to do on this?

Did a light review

Test coverage appears to be there
Post update calls for new config.

But patch 124 does not apply to latest development branch.

Also change record will need slight updating for branch version.

straight reroll of 124 for 11.x

looks like the conflicting test changes need some attention

Patch #124 doesn't apply on Drupal 10.2

I appreciate if someone can create the updated patch for Drupal 10.2.
Thanks!

Reroll of 124 for 10.2.x.

@mahde, thanx.

Looks like patch 128 works fine for 10.2.1

Thank you, @mahde!
Patch #128 works fine for 10.2.2 - PHP 8.2

Thanks @mahde! Can also confirm that #128 works for 10.2.3!

Experiencing some sort of strange issue with the remote so I'll just push up the updated 11.x patch.

Missing two use statements in content_moderation.module, which was causing error on node create forms.

Updated patch for 11.3.x.