Add a mark when editing a field property will affect all the translations as we do with fields themselves

Closed https://www.drupal.org/node/2582479 as a duplicate of this.

If credit can be moved over for.
peterarnold
jhodgdon
.

@smustgrave, thanks. I looked at the other issue as well. Adding credit.

Tagging because this was looked at as part of triaging the closed duplicate.

hi All,

I have re-rolled the patch so it works on the 9.5 branch,
added reroll patch as well

I also conducted a test with the below steps

• clean install drupal with standard profile
• enable content translation module
• added image field to the article page
• configure the image field to have translateable element on alt, and leave title non-translate able

below is the result.

config
before	after

it seems the patch is working fine, need another eye to review the latest patch.

thanks

fixing the command failed.

Patch #17 tested and Applied successfully on Drupal 9.5.x.

Before Patch:

After Patch:

Not sure all the screenshots are needed.

But can we get a tests-only patch and the full patch to verify a fail/pass.

Did not do a code review.

@awset thanks for your work on this issue. I have a few suggestion related to the code here

1. May be can combine the two if statements into a single one

// Elements are considered to be non-multilingual by default.
  if (!empty($element[$key]['#multilingual'])) {
  // The field is multilingual, but some property may not be.
  if (isset($field_definitions[$key])) {

2. We should be avoiding \Drupal calls within a class, we should be using dependency injection wherever possible

 $this->fieldTypeManager = \Drupal::service('plugin.manager.field.field_type');

3. Since $form_object is not getting utilised again in the code we can reduce

/** @var \Drupal\Core\Entity\ContentEntityForm $form_object */
$form_object = $form_state->getFormObject();
/** @var \Drupal\Core\Entity\ContentEntityInterface $entity */
$entity = $form_object->getEntity();

to
$entity = $form_state->getFormObject()->getEntity();

Addressed issues in #20

But this still needs tests

Hello, awesome ppl.

I have created a test-only patch and the test script as well, interdiff is attached too.
need review.

thanks.

it seems there is an unrelated error on the test.

the unrelated issue seems already fix it, need review.

Tested on Drupal 10.1.x and php 8.1

- The patch applies cleanly
- Issue summary is clear and explains the problem
- Test is included and test only patch is provided
- Couldn't identify any issues with the code
- Patch fixes the issue.

Not including the before / after screenshots here to avoid duplicates. Marking this issue RTBC

1. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -119,8 +128,10 @@ class ContentTranslationHandler implements ContentTranslationHandlerInterface, E
   +   * @param \Drupal\Core\Field\FieldTypePluginManagerInterface $field_type_manager
   +   *   The field type plugin manager.
       */
   -  public function __construct(EntityTypeInterface $entity_type, LanguageManagerInterface $language_manager, ContentTranslationManagerInterface $manager, EntityTypeManagerInterface $entity_type_manager, AccountInterface $current_user, MessengerInterface $messenger, DateFormatterInterface $date_formatter, EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository) {
   +  public function __construct(EntityTypeInterface $entity_type, LanguageManagerInterface $language_manager, ContentTranslationManagerInterface $manager, EntityTypeManagerInterface $entity_type_manager, AccountInterface $current_user, MessengerInterface $messenger, DateFormatterInterface $date_formatter, EntityLastInstalledSchemaRepositoryInterface $entity_last_installed_schema_repository, FieldTypePluginManagerInterface $field_type_manager) {
   

   We adding a new dependency we need set the default value to NULL and then trigger a deprecation notice if it is NULL and get the value from \Drupal::service().

2. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -625,6 +639,68 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
   +    // @todo Find a more reliable way to determine if a form element concerns a
   +    //   multilingual value.
   +    if (!isset($ignored_types)) {
   +      $ignored_types = array_flip(['actions', 'value', 'hidden', 'vertical_tabs', 'token', 'details', 'link']);
   +    }
   

   This should link to an issue on drupal.org... also what happens if this list is wrong.

3. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -625,6 +639,68 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
   +    $display_translatability_clue = !$entity->isDefaultTranslationAffectedOnly();
   

   I think we should do an early return here since we don't do anything if this is false...
   ie.

   if ($entity->isDefaultTranslationAffectedOnly()) {
     // Some explanation of why...
     return;
   }
   

4. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -625,6 +639,68 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
   +    $display_warning = FALSE;
   

   This is never set to TRUE.

5. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -625,6 +639,68 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
   +    // We use field definitions to identify untranslatable field widgets to be
   +    // hidden. Fields that are not involved in translation changes checks should
   

   The first sentence here seems to imply we're going to hide something. I can't see where and if that actually happens.

6. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
   @@ -625,6 +639,68 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
   +    if ($display_warning && !$form_state->isSubmitted() && !$form_state->isRebuilding()) {
   +      $url = $entity->getUntranslated()->toUrl('edit-form')->toString();
   +      $this->messenger->addWarning($this->t('Fields that apply to all languages are hidden to avoid conflicting changes. <a href=":url">Edit them on the original language form</a>.', [':url' => $url]));
   +    }
   

   I think this code can be removed. $display_warning is never set to TRUE.

7. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationLanguageChangeTest.php
   @@ -165,4 +165,50 @@ public function testTitleDoesNotChangesOnChangingLanguageWidgetAndTriggeringAjax
    
   +  public function testLabelMarkWhenEditingFieldProperty() {
   

   This method should have a comment saying what is being tested.

8. +++ b/core/modules/image/src/Plugin/Field/FieldWidget/ImageWidget.php
   @@ -234,6 +234,8 @@ public static function process($element, FormStateInterface $form_state, $form)
   +        '#for_column' => 'target_id',
   +        '#for_column_group' => 'file',
   

   This addition to the Field Widget API needs documentation somewhere. And would need a change record.

Reading the issue summary I was expecting all the logic to be in the field widget, and to be honest I think requiring the field widgets to handle this seems like a better approach than trying to do it semi-magically, especially given there are changes needed in the image widget anyway.

Didn't review yet, just a note that widgets like paragraphs tend to require special handling for stuff like this, also needed it for the existing message and we'll need to make sure that is possible. Will review after vacation

@catch asked me to take a look as well. I agree the field widget may use any way to display text, it could be in twig templates, JS, etc. We can's just assume that manipulating the render array will work with arbitrary widgets IMHO.

I've tried to fix the below points from comment #28 please review.

1. Added deprecation notice, added @see the tag as the current issue.
3. Added early return.
4. Removed variable $display_warning.
5. Removed unrelated comments.
6. Removed unnecessary code.
7. Added description for method.

Keeping the status to needs work for point number 2 and 8 of comment #28.

Try to fix CS errors.