ContentModerationState 'workflow' column empty for all non-default translations.

I guess we need a test for this?

Even though we are still an alpha experimental module we don't need an upgrade path (although I'd love one).

we're testing that when adding a translation the workflow column is set.

I was just thinking that previously it was being set to NULL and we didn't know because there was no test coverage.

Well, #2873287: Dispatch events for changing content moderation states provides an implicit test to prevent this from regressing in the future... I'd agree it seems a bit odd to re-test entity api and translation here...

ok, accepting the arguments in #7 and #9 lets RTBC.

I don't see any reasoning above on making the workflow not-translatable. While it may not be the 80% use-case, it seems totally valid to have different workflows for different translations of content. Especially in a context where you have an actual editorial + translation workflow. You might have an "editorial" workflow for the source content, but then have a "translation" workflow for each of the translations.

Maybe supporting that would be overly complex, but would like that verified first.

I think having a different workflow for languages for a given entity/bundle would be a new feature, since that isn't currently possible. I enabled the config translation module and attempted to translate a workflow, but all that can be changed is the labels, none of the entity/bundle to which it applies was available to translate.

Given that, we probably do want a test here so that it's explicit that the aforementioned feature doesn't yet exist. I'll see if I can pull a test together.

Without querying the sql table directly during the test, I wasn't able to find a way to readily test this in the tests themselves. However, adding an assertion to the entity operations class where these gets loaded nicely demonstrates the current bug, and also indicates that different workflows per-language is currently not supported.

I agree with #11 that a different workflow per language could definitely be a use case. However I also agree that this may be overly complex, and for that reason maybe something we don't want to support. Supporting that also makes this issue a little more complex to fix.

Awesome to see that @jhedstrom was able to find a way to test this!

I still stand by my RTBC and support the patch in #13. However leaving this as Needs Review for more discussion about different workflows per language.

#13 is definitely awesome, I will try to find some time to make that pass, while still keeping the field translatable. Depending on how much effort that is, I think we can then decide whether it makes sense to support it or not. Is that fair?

Having considered this for a few days, I don't think the assertion I've added is the right way to go if we want these to be configurable per-language. With this assertion in place, even if core doesn't provide that UI, contrib could, and this would prevent that from working.

Instead, we should probably just query the database records directly in the DefaultRevisionStateTest::testMultilingual() to demonstrate this current issue, and whatever fix is put in place.

Here's a patch with the alternate test.

I think the way to fix this while keeping the workflow flexible per-language would be to set the workflow value to the default language's workflow if it isn't already set. I'm not sure what the best practice on that is for translations, but this can't be the first place in core that's needed... I spent a bit of time trying preSave, as the node module uses that to set revision owners, but that didn't seem to work.

This should be enough to support per-translation workflows (at least on a storage level). So from my perspective the effort is reasonable to further support this use-case. The interdiff is relative to the TEST-ONLY patch in #19 and the patch contains those same tests and will (hopefully) be green.

+++ b/core/modules/content_moderation/tests/src/Kernel/DefaultRevisionStateTest.php
@@ -104,6 +109,32 @@ public function testMultilingual() {
+    $records = Database::getConnection()->select('content_moderation_state_field_revision', 'cms')
+      ->fields('cms', ['moderation_state', 'workflow'])
+      ->condition('content_entity_revision_id', $revision_id)
+      ->condition('langcode', $langcode)
+      ->execute()
+      ->fetchAll();
+    $this->assertCount(1, $records);
+    $record = array_pop($records);
+    $this->assertEquals($expected_state, $record->moderation_state);
+    $this->assertEquals($expected_workflow, $record->workflow);

I think this should use an entity query instead.

Something like (untested):

$moderation_state_storage = $this->entityTypeManager->getStorage('content_moderation_state');

$query = $moderation_state_storage->getQuery();
$results = $query->allRevisions()
  ->condition('content_entity_revision_id', $revision_id)
  ->condition('langcode', $langcode)
  ->execute();
$this->assertCount(1, $results);

$revision_id = key($results);
$moderation_state = $moderation_state_storage->loadRevision($revision_id);
$this->assertEquals($expected_state, $moderation_state->get('state')->value);
$this->assertEquals($expected_workflow, $moderation_state->get('workflow')->target_id);

This looks like a good solution.

Although I fear if a translation is added without going through EntityOperations::updateOrCreateFromEntity() then we'd still have the problem. One solution there would be to extend \Drupal\Core\Entity\ContentEntityBase::addTranslation within ContentModerationState?

I was unable to get entity query to work when I tried previously. However, we could change the test just to assert that there are no rows with a null langocde at the end as @Sam152 suggested above in #21.

OK, switched the test to an entity query. The dummy code in #23 didn't quite work, but almost. Also took the liberty of updating the docs to make them (to my mind) a bit clearer.

Not really sure, what to respond to #25. It seems if someone is manually creating translations of content moderation state entities they know what they're doing and can provide the workflow value themselves? That would be my gut reaction, but - like I said - not really sure...

ok, let's do this then!

#13 looks much more straightforward to me.

If contrib wanted to support workflow-per-translation could they not alter the field definition to do that?

No, if a field already has data that's not possible, at least not as far as I know.

Re #31: My reasoning is the following: Having per-entity workflows is already something that we don't support through the UI, but it's something that the data model we* have chosen for Content Moderation (i.e. the dedicated content moderation state entities) more or less required us to do, or at the very least it made the code much nicer to read, to fetch the workflow directly from the state, instead of going through hoops to fetch the bundle, etc. So depending on one's viewpoint it's already either a denormalization of the per-bundle workflow information or a "hidden feature" that would make it significantly easier for a contrib module to implement per-entity workflows than it otherwise would.

Now if you argue for the former I can see how fixing this bug with the minimal possible amount of complexity/code is desirable and that would probably be #13. But I would strongly argue in favor of the latter point: That without actually going through hoops we are enabling contrib do to fancier stuff. And per that viewpoint we are currently (i.e. in HEAD) enabling contrib to do - not only per-entity - but per-entity-translation workflows. And #27 fixes this bug, without removing that capability.

Additionally, I think #27 still couldn't be considered as "jumping through hoops" to enable a contrib use-case. I can see how it's slightly less nice from a pure code complexity standpoint than #13, but I don't think we are increasing complexity or maintenance cost or anything here, at least not in any significant manner.

(Note: I'm using "we" as "all of us/the community" here, I personally didn't actually write any of the Content Moderation code, nor was I significantly involved otherwise. Not sure if that makes sense, not a native speaker here, sorry...)

#13 leads to a harder upgrade path, and becuase it's experimental there will be no upgrade path, adding yet another thing for some people (@Sam152) to deal with. For that reason alone #27 looks like a nice solution. It's not like #27 adds loads of new API layers either, it's quite a neat solution. #27 is not as fool proof as #13, but maybe we need to worry about the fools when they come along.

If a field already has data is it possible to change it from non-translatable to translatable?

No, if a field already has data that's not possible, at least not as far as I know.

Actually, it is totally possible and it does not even require an update. Just switch the value and save the definition and you're done.

It would be nice if core had no bugs preventing people from having different workflows per language, but I'm not sure this is the kind of use case we need to support in core.

Yep, obviously when you do the switch only the original language has values, but you can easily implement a custom logic to fall back to the "default language" workflow if none is available for the current language.

Actually, it is totally possible and it does not even require an update. Just switch the value and save the definition and you're done.

Oh, sorry for my misinformation in that case, I wasn't aware of that. And thanks @plach, for setting me straight!!

Glad to still be useful from time to time :)

@tstoeckler would you support the patch in #13? (or even #19 with the updated test).

Well, I don't really want to hold it up, especially since it's not something that I won't be able to workaround in contrib/custom code. It just seems to me very unfortunate to require people that want per-translation workflows to have to employ some workaround - be that in a custom module or installing a dedicated contrib module for that - while people that want per-entity workflows can just go ahead and build a UI on the core API even though neither are supported by the core UIs. Unfortunate in particular because it requires so little code to support/fix.

FWIW, I would favor #27 over #13 mostly for the points @tstoeckler mentions in #41.

To try and get this issue back on track and stop the bike shedding lets look at the pros and cons of the two options:

1. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -61,7 +61,6 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -      ->setTranslatable(TRUE)
   

   Known as #13.
   Advantages:
   - Simple change.
   - Effective API wide.
   - Easy to update / change in contrib.
   Disadvantages:
   - Needs an update / change (workaround) in contrib (or custom) to allow per translation workflows.

2. +++ b/core/modules/content_moderation/src/EntityOperations.php
   @@ -192,7 +192,9 @@ protected function updateOrCreateFromEntity(EntityInterface $entity) {
   -        $content_moderation_state->addTranslation($entity_langcode);
   +        $content_moderation_state->addTranslation($entity_langcode, [
   +          'workflow' => $content_moderation_state->get('workflow')->target_id,
   +        ]);
   

   Known as #27.
   Advantages:
   - No API change.
   - Contrib or custom modules just need to implement a new UI to have per translation workflows. (as @tstoeckler mentions in #41 but as @Sam152 says in #43 how do we go about that? it seems more complex than it sounds)
   Disadvantages:
   - Only kicks in if going via EntityOperations::updateOrCreateFromEntity().

After reviewing the options I am in favour of what we've been referring to as #13, but the latest patch in #43 has better testing for this. It seems much simpler for contrib/custom modules to override, which is what @tstoeckler is arguing for. Also I believe we could always make it translatable in the future in a BC way, where as making it non-translatable in the future would be a BC break.

Leaving as Needs Review for now, but will RTBC #43 in a few days if there are no objections.

Also, please correct me if any of my pros and cons are incorrect.

As mentioned in #46, I'm RTBCing the patch in #43 because it fixes the issue and is easy for contrib to override.

Sorry, but I feel like I need to rectify some things. Since I'm only slightly annoyed by this issue and it's not a huge deal, I would have let this go, but since #46 explicitly mentions me I think I need to clarify my position.

- Contrib or custom modules just need to implement a new UI to have per translation workflows. (as @tstoeckler mentions in #41 but as @Sam152 says in #43 how do we go about that? it seems more complex than it sounds)

The parenthesis is pretty unfair and is a mis-statement of my earlier point. My point was that - in the data model - we already have per-entity workflows, even though that is not visible in the UI. I.e. we already have a separation of the UI concepts - which strictly enforce to have the same workflow for all entities of a bundle - and the data storage. My point is that since we already have that separation anyway, for reasons completely irrelevant to this issue, it is very reasonable to take this separation just an inch further to have per-translation workflows instead of per-entity workflows.

Providing a UI for either per-entity or per-translation workflows is certainly a daunting task, but the outcome of this issue doesn't change that, so the way the quote is states is misleading, in my opinion.

- Only kicks in if going via EntityOperations::updateOrCreateFromEntity().

While this is true, technically speaking, I have yet to hear any actual reason this is a problem. In fact we are actively removing any direct access points to the content moderation state entities currently in core (#2843753: Prevent ContentModerationState from being exposed by REST's EntityResource, #2894479: Deprecate the Views relationship from moderated content to the "content_moderation_state" entity ), so bringing up the possibility of manually creating content moderation state translations strikes as a bit inconsistent at least. In fact, if one were to experiment with a per-translation workflows UI in contrib, one would do exactly that, and then one would expect to have to explicitly specify the given workflow. So I don't really see this as a striking argument.

To me the only real argument is the added maintenance burden of the code added here, but - as also stated above - as its really very few lines of code and really just an extension of the pre-existing complexity that is inherent with such an internal/hidden entity type, I think the benefits outweigh this downside.

It seems much simpler for contrib/custom modules to override, which is what @tstoeckler is arguing for.

Sorry, but can you explain that? The way I read it this is a pretty clear untruth, but I'll give you the benefit of the doubt that I'm simply not getting what you mean by that.

Right, that's my understanding @Sam152.

My statement "It seems much simpler for contrib/custom modules to override, which is what @tstoeckler is arguing for." was trying to summarise that we're looking to allow contirb to have per translation workflows. It seems like the easiest way to do this, is implement hook_entity_base_field_info_alter and make the field translatable.

I don't mean to annoy, upset, or be unfair, and to be honest I don't really care too much I just want this issue fixed.

This issue is currently blocking #2873287: Dispatch events for changing content moderation states.

Ahh, I think I understand now what you are getting at.

With the alternative implementation, I don't see as clearly how you'd hook into this and prevent the workflow field from being set in the new translation in the first place.

So basically you are saying it is easier to make the field translatable from contrib (if it is untranslatable in core) than to make it untranslatable (if it is translatable in core)?

I'm not sure, maybe @plach has more info on that, but I don't necessarily think that's the case. Since there's no table layout change either way, it seems it would work either way and would be equally simple either way?

So I think that really depends on what your trying to do. Since the workflow that is being set here, is determined at the top of the function by

$workflow = $this->moderationInfo->getWorkflowForEntity($entity);

you can have a workflow plugin that determines the workflow based on the language or its translation status, etc.

Edit: Sorry, my brain mixed up some similar looking lines, sorry, proper response coming up...

Right, so yeah, I do think it really depends on what you're trying to achieve.

Generally, since this code is invoked from hook_entity_insert() and hook_entity_update() you can implement the same hooks and - depending on whether you run before or after (which you can control reliably (albeit not elegantly) by setting the module weight) - either create a content moderation state translation before this code runs - in this case the (pre-existing) ->hasTranslation() will respect your choice and not do any manual processing - or after in which case you can simply override the workflow to your desired value and re-save the content moderation state translation.

If what you are actually trying to do is have some deterministic logic behind which entity receives which workflow, you probably want to swap out the ModerationInformation class and in particular its getWorkflowForEntity() method. That is what you would be doing anyway in this case, and as the code snippet in #54 shows, the added code will respect this, as well.

OK, so we have that cleared up at least, that's good. Thanks @timmillwood and @Sam152 for clarifying your positions.

I wouldn't go so far as to call myself "happy", but I don't want to be in the way of this going in.

Moving this back to 8.4.x, this is a blocker for a "should have" issue which is needed for Content Moderation to become stable in 8.4.x

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

If we run up against issues supporting translation issues, then let's absolutely open those, but happy to see the simpler fix RTBC.