Make exposure of translation meta fields conditional

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 ✨ UI for publishing/unpublishing block_content blocks Needs work . 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 ✨ UI for publishing/unpublishing block_content blocks Needs work . 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 ✨ UI for publishing/unpublishing block_content blocks Needs work 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

The last submitted patch, 81: 2971390-81.patch, failed testing. View results →

The last submitted patch, 81: 2971390-81.patch, failed testing. View results →

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.

Seems this also needs a reroll for 10.3.x! Patch #81 doesn't apply any more.