Make exposure of translation meta fields conditional

Setting to needs work for tests.

On the handler, we have things like \Drupal\content_translation\ContentTranslationHandler::hasAuthor() and and hasPublishedStatus() + hasCreated(). This hardcoded logic probably predates those generic checks and field definitions by a long time.

Maybe we can use those to dynamically define those fields only if necessary. We then likely also need to update the submit logic to not call setAuthor() and so on if we don't have the field exposed. Not quite sure how that even works right now to be honest. I can imagine the value is actually replaced anyway.

Looks quite nice to me, lets get some feedback from @plach about the direction before we continue with it.

1. +++ b/core/modules/block_content/src/BlockContentTranslationHandler.php
   @@ -5,12 +5,26 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function entityFormAlter(array &$form, FormStateInterface $form_state, EntityInterface $entity) {
   +    parent::entityFormAlter($form, $form_state, $entity);
   +
   +    if (isset($form['content_translation'])) {
   +      // Block content entity does not expose the status field. Make content
   +      // translation status field visible instead.
   +      $form['content_translation']['status']['#access'] = TRUE;
   +    }
   +  }
   +
   

   This is actually a temporary state:

   #2834546: UI for publishing/unpublishing block_content blocks

   We should at least add a todo or even consider to postpone..

2. +++ b/core/modules/comment/src/CommentTranslationHandler.php
   @@ -41,7 +26,6 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
          $translation = &$form_state->getValue('content_translation');
          /** @var \Drupal\comment\CommentInterface $entity */
          $translation['status'] = $entity->isPublished();
   -      $translation['name'] = $entity->getAuthorName();
        }
   

   why can we only remove the name but not the status logic?

3. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationMetadataTrait.php
   @@ -0,0 +1,74 @@
   +  protected function hasMetadataField($field_name) {
   +    $has_metadata_field = array_key_exists($field_name, $this->fieldStorageDefinitions) && $this->fieldStorageDefinitions[$field_name]->isTranslatable();
   +    // Check if the entity type implements EntityOwnerInterface for uid field.
   +    if ($field_name === 'uid') {
   +      return $has_metadata_field && $this->container->get('entity_type.manager')->getDefinition($this->entityTypeId)->entityClassImplements(EntityOwnerInterface::class);
   +    }
   +
   

   you're actually calling $this->container here anyway, is that just to avoid repeated calls? IMHO not worth it for a test helper.

   you can also use isset($this->fieldStorageDefinitions), array_key_exists() is only necessary when the value of the array key could be NULL

4. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
   @@ -49,6 +50,15 @@
    
   +  /**
   +   * {@inheritdoc}
   +   */
   +  protected function setUp() {
   +    parent::setUp();
   +
   +    $this->fieldStorageDefinitions = $this->container->get('entity_field.manager')->getFieldStorageDefinitions($this->entityTypeId);
   +  }
   +
   

   I think this is a rather strange pattern, I know some don't agree but IMHO it is fine to just use \Drupal:: in test code, then you don't need to implement that setUp() method multiple times.

5. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestMulChanged.php
   @@ -55,6 +55,12 @@ class EntityTestMulChanged extends EntityTestMul implements EntityChangedInterfa
    
   +    // Show the created field in the edit form.
   +    $fields['created']->setDisplayOptions('form', [
   +      'type' => 'datetime_timestamp',
   +      'weight' => 10,
   +    ]);
   +
        $fields['changed'] = BaseFieldDefinition::create('changed_test')
   

   those additions are necessary because we test that they are shown/not shown in the generic test?

Setting back to needs work to resolve the issues raised in #16, we should also try to point this issue at @plach for that review.

Reroll of #14 - manually fixed conflicts on:

• core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
• core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php

Since these moved to using getAccountName()

Triggering test bot by setting to needs review, but this still needs work (see #17).

Helps if i attach the patch...

Oops!

I'm new to the patch, but in the hope of pushing this forward here is a bit of work on it:

Re #15:
1. I've added a @TODO for now, hopefully the other issue lands earlier than this.
2. I think we can remove this indeed. Done.
3.

you're actually calling $this->container here anyway, is that just to avoid repeated calls? IMHO not worth it for a test helper.

Not entirely sure what you mean, left as is.

you can also use isset($this->fieldStorageDefinitions), array_key_exists() is only necessary when the value of the array key could be NULL

Done.
4. I have no opinion one way or the other, so left it as is.
5. That would be my guess, left it as is.

Seems like we do need to use array_key_exists()

Rerolled, manually fixed conflicts on :

• core/modules/content_translation/src/ContentTranslationHandler.php
• core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
• core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php

1. +++ b/core/modules/block_content/src/BlockContentTranslationHandler.php
   @@ -14,6 +15,21 @@ class BlockContentTranslationHandler extends ContentTranslationHandler {
   +    if (isset($form['content_translation'])) {
   +      // @TODO this will not be necessary once this issue lands:
   +      // https://www.drupal.org/project/drupal/issues/2899923
   +      // Block content entity does not expose the status field. Make content
   +      // translation status field visible instead.
   +      $form['content_translation']['status']['#access'] = TRUE;
   +    }
   

   Formatting for @todo is not correct, I'd put it below the explanation, lowercase and then the second line needs to be indented by two spaces.

   Also, the issue seems to be wrong, might be my fault, that's the issue for taxonomy status, there should be a separate one for block_content.

2. +++ b/core/modules/block_content/tests/src/Functional/BlockContentTranslationUITest.php
   @@ -125,6 +125,19 @@ protected function getEditValues($values, $langcode, $new = FALSE) {
   +  protected function hasMetadataField($field_name) {
   +    $result = parent::hasMetadataField($field_name);
   +    // Block content entities has status field but it does not expose it.
   +    if ($field_name === 'status') {
   +      return FALSE;
   

   this should get the same @todo so we don't forget to remove it.

3. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
   @@ -57,6 +59,15 @@
   +
   +    $this->fieldStorageDefinitions = $this->container->get('entity_field.manager')->getFieldStorageDefinitions($this->entityTypeId);
   +  }
   

   what I mean is that we should drop $this->fieldStorageDefinitions, just get them directly in the trait, also use \Drupal::service() there.

4. +++ b/core/modules/content_translation/src/Tests/ContentTranslationUITestBase.php
   @@ -323,16 +336,15 @@ protected function doTestAuthoringInfo() {
   +      $edit += $this->getMetadataValueAndAssertNoField('created', format_date($values[$langcode]['created'], 'custom', 'Y-m-d H:i:s O'));
   

   format_date() is deprecated, that seems to be the reason for all the test fails.

Thanks for the review @Berdir!

#30.1 Great catch on the wrong issue, fixed that and the formatting. Linking now to #2834546: UI for publishing/unpublishing block_content blocks
#30.2 Added.
#30.3 Got it, done.
#30.4 Fixed.

Forgot to remove the setUp from \Drupal\Tests\content_translation\Functional\ContentTranslationUITestBase, sorry test bot...

Re #15.2 - On #12.2 its mentioned that the title could be a leftover that could be removed. However if we remove the line for the status, seems like the status isn't saved. NodeTranslationHandler::entityFormEntityBuild() has the same logic so I believe we need that in place, so re-adding it here.

Various improvements:

* terms need the same hack now as block_content, this is pretty sad, will talk to @amateescu about this as well if we can provide a more generic solution for this. Maybe check the form display configuration of the field definition?
* Improved the generic test logic to handle widget form elements like status[value] and uid[0][target_id]
* Improved entityFormEntityBuild() to only set the value if the form element is accessible, that works also for the block_content and term cases.
* With that fix, we actually can remove the overrides in node and comment, because the parent doesn't try to override the values anymore.

Discussed a bit with @Berdir and my opinion is that CT should alter the form display for publishable entity types and expose their status field rather than doing it with form alters :)

Rerolled. term status is now exposed, so that's one less to worry about.

Another reroll.

Updated the patch since #2899923: UI for publishing/unpublishing taxonomy terms got in

Reroll for 9.0,x, only change is the removed old base test class.

This would be a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!

Whao, thank you all very much, this solved a heavy bug for us after #2899923: UI for publishing/unpublishing taxonomy terms which made it impossible to unpublish commerce product variation translations through the UI! While the core status checkbox could be unchecked in the translation, the content_translation checkbox was disabled="disabled" and could not be changed. That lead to overridden value for the status checkbox. So the status could not be changed to disabled anymore.

In #2899923#76 I posted a screenshot of a further case, where the published status is also affected by this and can not be changed due to the bug:

Top checkbox is disabled, can not be changed but overrides the checkbox below.

We're using #46 in 8.8.6 in production on a large site since some days and I can finally confirm it works very well and without any problems. Thank you so much.

So RTBC+1 for #46 on 8.x. Would be very helpful to also fix this in 8.9.x as follow-up fix for #2899923. I don't see this as a new feature, but much more as an important bugfix for a broken UI as described above.

Hi @Berdir & @xjm, is this ready for RTBC? And do we have a chance to backport this as written in #49 to also fix 8.x installations where this bug is also still unfixed?

It is tagged as "Needs issue summary update". I would go through the patch after the IS is updated to reflect the taken decisions.

Reroll.

Updated deprecation notices.

Thanks for the updates. The changes to ContentTranslationWorkflowsTest that are failing are pretty new, so that makes sense. Another (test) entity type with "inconsistent" field definitions (a status field without form display configuration) though, that is a bit trickier, Will get back to that in a follow-up comment.

So, this patch does have basically two effects:

a) Prevents duplicate author/created/status fields from showing up if already exposed as widgets in the given entity form. Some entity types like node and comment have custom handlers in place to hide those fields, but most entity types don't, even in core. For example taxonomy terms and media. So this causes a change for those entity types but I think that might be acceptable in a minor release. The duplicate fields are very confusing and cause bugs, like overwriting the actual field widgets.

b) The logic for showing/hiding those extra fields automatically is however tied to just whether the field exists or not and not if it's visible in the form. For entity types that have for example a status field but don't show it in the UI (like block_content and test entities in core), that results then in neither the regular status field nor the content translation status field being exposed. To keep the current behavior in e.g. block_content, that results in having to override it to show those fields anyway and some entity types need form display configuration for our translation tests to work.

Like this:

+++ b/core/modules/block_content/src/BlockContentTranslationHandler.php
@@ -5,12 +5,28 @@
+
+    if (isset($form['content_translation'])) {
+      // Block content entities do not expose the status field. Make the content
+      // translation status field visible instead.
+      // @todo This will not be necessary once this issue lands:
+      //   https://www.drupal.org/project/drupal/issues/2834546
+      $form['content_translation']['status']['#access'] = TRUE;
+    }
+  }

Question is: How much change is allowed and how strict do we need to be with BC with these changes? Form structures are not included in our BC promise, but what about form structures by a default implementation of an entity handler?

Options:

1) This is fine, we're OK with the change and move forward. Hopefully block_content will eventually expose the status field and we can remove these overrides again.

2) We check not whether an entity type has a field but also if it is enabled in the current form display. I don't like this, seems very unpredictable. Maybe you don't need the node author/created field on a node type, hide it and then instead one pops up from content_translation and only on translations? Yep, weird.

3) We make the new behavior strictly opt-in, with a new base class maybe or a entity type definition flag. You need to explicitly set that in D9 and in D10 it becomes the default and the old class is removed (or the new opt-in flag removed). That would mean no unexpected changes happen to your entity type, but a lot of contrib/custom entity types would then also not benefit from these imho rather important UX fixes unless they explicitly change something in their code. Not in D9 anyway. And they would only get deprecation messages if they have explicit content_translation test coverage. If they would, they'd have already customized this.

Thoughts?

Another reroll due to test conflicts.

Another one.

Another reroll, this time conflicted on PHP 8.1 related changes in EntityTestMulRevPub.php.

Thanks @Berdir! Just wanted to inform that we're using the patch(es) successfully since #46. So far RTBC +1.

Sadly I'm not experienced enough in this topic to decide on #58, Who could?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

git apply -3 still appllied fine.

For the issue summary update.

Haven't done a review yet.

I updated the summary

small fix in the summary about the blocks published state

Confirmed the issue on a standard install
Adding a 2nd language for the Image media bundle
Applied patch #68 and am no longer seeing the duplicates

Thanks, really hoping this will finally land :)

+++ b/core/modules/block_content/src/BlockContentTranslationHandler.php
@@ -5,12 +5,28 @@
 
+  /**
+   * {@inheritdoc}
+   */
+  public function entityFormAlter(array &$form, FormStateInterface $form_state, EntityInterface $entity) {
+    parent::entityFormAlter($form, $form_state, $entity);
+
+    if (isset($form['content_translation'])) {
+      // Block content entities do not expose the status field. Make the content
+      // translation status field visible instead.
+      // @todo This will not be necessary once this issue lands:
+      //   https://www.drupal.org/project/drupal/issues/2834546
+      $form['content_translation']['status']['#access'] = TRUE;
+    }
+  }
+

As mentioned before and also in the issue summary. this part is a bit weird and an edge case between a change and a bugfix.

This is keeping the current status, which is arguably strange as the default translation has no status, but the translations then do.

We could also say it's a bugfix, there shouldn't be a translation status, but people might have unpublished translations and then they couldn't publish them anymore.

I think this is very unlikely to happen to any other contrib/custom entity type, either they have a status, and then also a UI or not.

I'm triaging RTBC issues.

I read the IS and comments. I did not find unanswered questions. Both #15 and #22.1 question whether this should be postponed on #2834546: UI for publishing/unpublishing block_content blocks. There wasn't any discussion about that. Should this be postponed? I've added this question to the remaining tasks.

I read the comments in the patch and had the following thoughts. Since this is testing against 10.1 instead of 11.x I will make a new patch for item 2 below.

1. +++ b/core/modules/block_content/src/BlockContentTranslationHandler.php
   @@ -5,12 +5,28 @@
   +      // @todo This will not be necessary once this issue lands:
   +      //   https://www.drupal.org/project/drupal/issues/2834546
   

   It would be more helpful in the @todo gave a hint at what to do here. It is to remove that next line? But then what is the purpose of the form alter? And I guess this is not needed if the other issue is completed first, which goes back to the postponing question.

   This applies to the other instances of the @todo as well.

2. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationMetadataTrait.php
   @@ -0,0 +1,77 @@
   + * Provides helps to assert content translation meta data fields.
   

   A bit of incorrect grammar here I think. How about "Provides methods to assert ..."

I don't think the question about postponing and #75.1 block this from RTBC. But I really shouldn't restore RTBC because of the edit I made to a comment.

> Both #15 and #22.1 question whether this should be postponed on #2834546: UI for publishing/unpublishing block_content blocks. Should this be postponed? I've added this question to the remaining tasks.

:shrug:

It would simplify the patch, but we've been soft-blocked by that issue for 5 years now. It's classified as a feature, this is a bug that fixes some pretty annoying issues for multilingual sites.

Nothing changes if we get this in compared to HEAD for blocks, we just need a bit of a workaround for it, which the other issue can easily remove again. The change that happens is technically also correct, if the module for some reason doesn't want to have a status visible, why should translations? We're just doing this for BC. Thinking this through, this whole concept maybe shouldn't exist at all, it's a leftover of Drupal 7 really where modules didn't have the ability to have translatable base fields/properties. Of course, no idea how to remove it now in a way that is backwards compatible.

IMHO the hidden concern is the last paragraph of #74, whether or not there are any custom or contrib entity types with a similar case, we would have never thought of that without the current state of block_content. IMHO it's fairly unlikely. I did just create a change record to notify about the UI changes and impacts on entity types: https://www.drupal.org/node/3383265.

The comment change in #75 is fine, the interdiff seems a bit confused about some line changes, but I think nothing changed there.

For what it's worth I am trying to get #2834546: UI for publishing/unpublishing block_content blocks into 10.2 or 10.3. But since there could be some back n forth on that one agree with @Berdir about moving this forward.

@Berdir, thanks for the explanation.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rerolled

All green from reroll. Restoring status

Seems unrelated

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Rerunning tests but patch still applies cleanly.

Found out why there's no MR

@smustgrave what does #89 mean? Or should it mean "Find" (active)

Created MR and rerolled.

Left some comments that I should of noticed before but appears to have phpcs errors also.

Thanks for picking this one back up.