[meta] Allow Paragraphs widget/field and similar use cases to to be considered translatable

Thanks for opening this, the suggested plan looks great to me at first sight:

* First [...] it would be easier if widgets could just set #multilingual themself and content_translation_form_alter() would not override it.

I vaguely remember to have briefly tried something similar and failed but +1 on this in principle :)

The original plan when porting Entity Translation to D8 was to make #multilingual an official key (see also #1498724: Introduce a #multilingual key (or similar) for form element), so whatever goes in that direction is welcome.

* Then we need to prevent the field from being replaced with the default revision/translation values [...] maybe we need [...] a hook that allows us to at least somehow undo things.

I already found a couple of use cases that would require the addition of an hook_entity_create_revision() hook (see #2940204-37: Translatable fields with synchronization enabled should behave as untranslatable fields with respect to pending revisions and #2941736: Moderation state revisions should have their isDefaultRevision() match the host entity's), so I think this could be the correct approach.

My main concern is that whenever we change a paragraph entity the parent reference is updated with the new revision, which causes all translations to be marked as affected (or only the default one, depending on the site settings). I think we also need a way to make sure that only the active translation is marked as affected. That may be achievable by forcing the RTA flag value via this new hook.

* While we don't want the paragraph field values to be replaced, we basically need to be able to apply the same/similar logic to our referenced entities. [...]

I think this could also be addressed by the hook_entity_create_revision() hook: if paragraphs iterated over referenced children and made the new revision objects available in the form, it should be possible to apply this logic recursively.

Nice!

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -254,6 +254,12 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
+      $this->moduleHandler->alter('entity_revision_skip_fields', $skipped_field_names, $new_revision, $context);

I'm wondering whether it would be possible to use EntityChangesDetectionTrait and move this alter hook over there (and statically cache the result).

Yep, statically caching by entity type / bundle of course. And then we can introduce the revision creation hook to create merged revisions for paragraphs.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -6,6 +6,7 @@
   +use Drupal\Core\Field\FieldItemListTranslationChangesInterface;
   

   No longer around :)

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -14,6 +14,8 @@
   +  use EntityChangesDetectionTrait;
   
   @@ -243,7 +245,7 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
   -      $skipped_field_names = array_flip($this->getRevisionTranslationMergeSkippedFieldNames());
   +      $skipped_field_names = array_flip($this->getFieldsToSkipFromTranslationChangesCheck($entity));
   

   Now that I had a closer look at this code, I think that I did not use EntityChangesDetectionTrait here previously because the meanings of ::getRevisionTranslationMergeSkippedFieldNames() and getRevisionTranslationMergeSkippedFieldNames are slightly different and I wasn't sure whether they would always share the same logic, should we have to update it in the future.

   However, it seems unlikely that they are currently sharing it by chance, so I'm wondering what's the common denominator between these two use cases:

   • when creating a new merged revision, it makes sense to skip any revision metadata, since it will be different in the new revision and it could even mess up things, e.g. copying the default revision ID would result in the wrong value to be returned by ::getLoadedRevisionId(); if we consider also the ERR use case, basically in this case we want skip any field that always varies by revision (is revision-specific);
   • when detecting revision translation changes, we want to skip revision metadata because we know that those may be different without indicating an actual change in the entity (translation) data; given that also the ChangedItem is involved in this case, I believe here we are more interested in a data vs metadata split, and revision-specific fields are normally metadata.

   So, maybe that's the only common bit? Both need to identify revision-specific fields to implement two distinct business logics?

3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -310,27 +318,6 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
   -  protected function getRevisionTranslationMergeSkippedFieldNames() {
   

   Per the previous bullet, I'd keep this method around, and just retrieve the result of ::getFieldsToSkipFromTranslationChangesCheck() from within that. I guess we will need to add an $entity parameter and make it optional to preserve BC. Or we could change ::getFieldsToSkipFromTranslationChangesCheck() to accept an $entity_type instead of an $entity, it's an @internal trait so this should be fine.

4. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
   @@ -400,4 +400,15 @@ public function equals(FieldItemListInterface $list_to_compare) {
   +    if (!$this->getFieldDefinition()->isComputed()) {
   +      if (!$this->equals($original_items)) {
   +        return TRUE;
   +      }
   +    }
   

   This could be simplified to return !$this->getFieldDefinition()->isComputed() && !$this->equals($original_items) :)

5. +++ b/core/lib/Drupal/Core/Field/FieldItemListInterface.php
   @@ -271,4 +271,17 @@ public static function processDefaultValue($default_value, FieldableEntityInterf
   +   * Allows to a field to return if it has changes that affect a translation.
   

   If this is going to be one of the first building blocks of a change detection/resolution API, maybe it would make more sense not to mention translation specifically, as this method can be used on any item to detect "actual" / relevant / data-affecting changes.

6. +++ b/core/lib/Drupal/Core/Field/FieldItemListInterface.php
   @@ -271,4 +271,17 @@ public static function processDefaultValue($default_value, FieldableEntityInterf
   +   * @param string $langcode
   +   *   The language that should be checked.
   ...
   +  public function hasAffectingChanges($langcode, FieldItemListInterface $original_items);
   

   Why do we need to specify a $langcode parameter? Can't we rely on the field language itself? Or the parent entity's active language?

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/LanguageSelectWidget.php
   @@ -30,6 +30,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $element['#multilingual'] = TRUE;
   

   By moving this key here we are making it an "official" core key, so I guess we should document it somewhere. As an alternative we could alter the widget itself from CT via hook_field_widget_WIDGET_TYPE_form_alter.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -307,6 +307,14 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
   +    $this->moduleHandler->alter('entity_revision_create', $new_revision, $entity, $context);
   +
   +
   

   Can we make this a regular entity hook like hook_entity_create(), thus also supporting the entity type-specific version?
   That is $this->invokeHook('revision_create', ...). We can add a $context param or implement dynamic argument forwarding.

   Also, extra blank line :)

2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -909,6 +909,25 @@ function hook_ENTITY_TYPE_create(\Drupal\Core\Entity\EntityInterface $entity) {
   + * Respond to entity revision creationg.
   

   typo

3. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -909,6 +909,25 @@ function hook_ENTITY_TYPE_create(\Drupal\Core\Entity\EntityInterface $entity) {
   + * This hook runs when creating a new revision.
   

   Not sure this adds much value to the previous line :)

   We could may mention the revision translation merge and this being the right place to mess with its logic?

   Or skip the line altogether :)

4. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -909,6 +909,25 @@ function hook_ENTITY_TYPE_create(\Drupal\Core\Entity\EntityInterface $entity) {
   + * @param \Drupal\Core\Entity\EntityInterface $new_revision
   + *   The new revision that was created.
   + * @param \Drupal\Core\Entity\EntityInterface $entity
   + *   The original entity that was used to create a revision from.
   + *
   

   Missing $context

5. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -909,6 +909,25 @@ function hook_ENTITY_TYPE_create(\Drupal\Core\Entity\EntityInterface $entity) {
   +function hook_entity_revision_create(Drupal\Core\Entity\EntityInterface $new_revision, Drupal\Core\Entity\EntityInterface $entity, $context) {
   

   Right now it's an alter hook, but I'd keep this way for consistency with hook_entity_create.

6. +++ b/core/lib/Drupal/Core/Field/FieldItemListInterface.php
   @@ -271,4 +271,24 @@ public static function processDefaultValue($default_value, FieldableEntityInterf
   +   * This is for example used to determine if a revision of an entity has
   +   * changes in a given translation. Unlike
   +   * \Drupal\Core\Field\FieldItemListInterface::equals(), this can report
   +   * that for example an untranslatable field, despite being changed and
   +   * therefore technically affecting all translations, is only internal metadata
   +   * or only affects a single translation.
   

   Perfect, we can expand on this if/when the method becomes useful in other contexts.

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/LanguageSelectWidget.php
   @@ -30,6 +30,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $element['#multilingual'] = TRUE;
   +
   

   I meant that we could do this as an alternative, if we don't want to make the key "officially part of core" yet:

   function content_translation_field_widget_language_select_form_alter(array &$element, FormStateInterface $form_state, $context) {
     $element['#multilingual'] = TRUE;
   }
   

8. +++ b/core/modules/content_translation/content_translation.module
   @@ -333,9 +333,14 @@ function content_translation_form_alter(array &$form, FormStateInterface $form_s
   +
   +        // Allow the widget to define if it should be treated as a multilingual
   +        // field, we need to move the key up.
   

   Extra blank line :)

Looks good!

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -308,12 +308,9 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
   +    $this->moduleHandler()->invokeAll($this->entityTypeId . '_revision_create', [$new_revision, $entity, $keep_untranslatable_fields]);
   +    // Invoke the respective entity-level hook.
   +    $this->moduleHandler()->invokeAll('entity_revision_create', [$new_revision, $entity, $keep_untranslatable_fields]);
   

   Minor can we store the arguments in variable and pass that to ::invokeAll()?

2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -910,19 +910,42 @@ function hook_ENTITY_TYPE_create(\Drupal\Core\Entity\EntityInterface $entity) {
   + * Respond to entity revision creation.
   ...
   + * Respond to entity revision creation.
   

   Sorry, missed that: "Responds" :)

3. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -910,19 +910,42 @@ function hook_ENTITY_TYPE_create(\Drupal\Core\Entity\EntityInterface $entity) {
   + *   The original entity that was used to create a revision from.
   ...
     *   The original entity that was used to create a revision from.
   

   "the revision from" would sound better to me.

4. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -910,19 +910,42 @@ function hook_ENTITY_TYPE_create(\Drupal\Core\Entity\EntityInterface $entity) {
   + * @param bool|null $keep_untranslatable_fields
   + *   Whether untranslatable field values should be kept or copied from the
   + *   default revision when generating a merged revision.
   ...
   + * @param bool|null $keep_untranslatable_fields
   + *   Whether untranslatable field values should be kept or copied from the
   + *   default revision when generating a merged revision.
   

   Maybe we could modified that to say "Whether untranslatable field values were kept or copied from the default revision when generating a merged revision." otherwise this seems prescriptive whereas all the work has been done and hook implementors are free to do what they need to.

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDecoupledTranslationRevisionsTest.php
   @@ -588,4 +588,39 @@ public function testRemovedTranslations() {
   +  public function testCreateRevisionHook() {
   

   Shouldn't this test be part of the CRUD entity test?

6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDecoupledTranslationRevisionsTest.php
   @@ -588,4 +588,39 @@ public function testRemovedTranslations() {
   +    $translation = $entity->addTranslation('it');
   

   :)

7. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDecoupledTranslationRevisionsTest.php
   @@ -588,4 +588,39 @@ public function testRemovedTranslations() {
   +    // Assert the data passed to the hook.
   

   Can we add assertions also for $keep_untranslatable_fields flag?

Based on #18, I'm changing this issue to needs work.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -308,9 +310,9 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
        // Allow modules to customize the created revision.
   

   This is only one use case: what about changing the comment to a more generic "Notify modules about the new revision"?

2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraintValidator.php
   @@ -116,14 +116,9 @@ protected function hasUntranslatableFieldsChanges(ContentEntityInterface $entity
          // nothing was actually changed. Thus, the changed time needs to be
          // ignored when determining whether there are any actual changes in the
          // entity.
   

   Nice clean-up, we can remove this comment now.

3. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraintValidator.php
   @@ -116,14 +116,9 @@ protected function hasUntranslatableFieldsChanges(ContentEntityInterface $entity
   -      if ($field instanceof ChangedFieldItemList) {
   

   We no longer need the use statement for ChangedFieldItemList.

4. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraintValidator.php
   @@ -116,14 +116,9 @@ protected function hasUntranslatableFieldsChanges(ContentEntityInterface $entity
   -      $items = $field->filterEmptyItems();
   +      $field = $entity->get($field_name)->filterEmptyItems();
          $original_items = $original->get($field_name)->filterEmptyItems();
   -      if (!$items->equals($original_items)) {
   +      if ($field->hasAffectingChanges($original_items, $entity->getUntranslated()->language()->getId())) {
   

   Variable names are a bit of a mess now :) Can we stick with $items / $original_items?

Re #20:

1: As we discussed, I think the approach of just forwarding what was passed is correct, otherwise we lose that information, while the default value can be computed again, although that means duplicating the related logic. We should probably explicitly document the expected default in the PHP docs, both in the hook and in TranslatableRevisionableStorageInterface::createRevision().
2: You're absolutely correct: https://www.drupal.org/node/1354#hooks :)
5: Nevermind, I thought we had a test covering all CRUD hooks, including multilingual ones.
6: Thanks for not being nice to me :))
7: Ok, still to do per bullet 1.

Re the #multilingual key, it sucks but I agree we need a test widget.

Nice, I think we are down to nits, the test failure seems trivial to fix.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
   @@ -307,6 +309,11 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
   +    // Notify modules about the new revision
   

   Missing trailing dot.

2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
   @@ -909,6 +909,51 @@ function hook_ENTITY_TYPE_create(\Drupal\Core\Entity\EntityInterface $entity) {
   + * @param bool|null $keep_untranslatable_fields
   + *   Whether untranslatable field values were kept or copied from the default
   + *   revision when generating a merged revision.
   

   This paragraph was not updated.

3. +++ b/core/modules/content_translation/content_translation.module
   @@ -333,9 +333,11 @@ function content_translation_form_alter(array &$form, FormStateInterface $form_s
   +        // Allow the widget to define if it should be treated as a multilingual
   +        // by respecting an already set #multilingual key.
   

   typo: "as multilingual"

4. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDecoupledTranslationRevisionsTest.php
   @@ -588,4 +588,39 @@ public function testRemovedTranslations() {
   +    // Assert the data passed to the hook.
   

   Can we assert also the keep_untranslatable_fields flag?

5. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDecoupledTranslationRevisionsTest.php
   @@ -588,4 +588,39 @@ public function testRemovedTranslations() {
   +    $data = \Drupal::state()->get('entity_test.hooks');
   

   We should able able to use $this->state here.

Looks great to me, I think we just need a CR now.

This seems to be a duplicate of #2826021: FieldItemList::equals is sufficient from the storage perspective but not for code checking for changes. I'm fine with closing that one instead of this one as this one is further along now in terms of test coverage, etc. But I would request 2 things here:

1. That @hchonov and @mkalkbrenner are credited here
2. That one of them sign off on this issue. We have a similar need for a slightly different use-case and it looks as though the API here is congruent with the one there, but I would like to have that confirmed before this goes in

+1 on #29.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1428,18 +1427,10 @@ public function hasTranslationChanges() {
   +      if ($items->hasAffectingChanges($original_items, $this->activeLangcode)) {
   

   This doesn't work for the default translation, as then $this->activeLangcode has the value of LanguageInterface::LANGCODE_DEFAULT. This has been solved in the similar issue already - #2826021: FieldItemList::equals is sufficient from the storage perspective but not for code checking for changes. Basically we need the following transformation:
   $langcode = ($this->activeLangcode == LanguageInterface::LANGCODE_DEFAULT) ? $this->defaultLangcode : $this->activeLangcode;

2. +++ b/core/modules/field/tests/modules/field_test/src/Plugin/Field/FieldWidget/TestFieldWidgetMultilingual.php
   @@ -0,0 +1,30 @@
   +    $elements['#multilingual'] = TRUE;
   

   Wouldn't be better to define this property in the field widget annotation and then take care of setting it in WidgetBase::form()? I would prefer this, as this way we kind of provide an official API for the property by having to introduce it in \Drupal\Core\Field\Annotation\FieldWidget.

@berdir said in slack:

We're not quite sure yet if we should split each change into a separate issue or keep them together like that, but I kind of like being able to provide an overview of the changes, because it is likely that someone interested in it is going to need more than just one of those new things

I don't know what a committer would decide, but the current issue is solving three different problems. I personally would separate the concepts and if desired we still might publish only one MR. If we do this, then we already have the issue which is introducing the similar method to FieldItemListInteface::hasAffectingChanges() - #2826021: FieldItemList::equals is sufficient from the storage perspective but not for code checking for changes, which means that we would need only one additional issue for the new hooks and the current issue might take care only of the #multilingual property.

@hchonov, #31:

1. Good catch, we will need test coverage for that.
2. Berdir and I were wondering about that as well, there is also a dedicated issue for it: #1498724: Introduce a #multilingual key (or similar) for form element. I think that should be our ultimate goal, however once we make the #multilingual key official, we need to take all the most common scenarios into account. For instance it may no longer be used only by the CT module, in fact it would make little sense for core to expose a key that's only used by one module. Hence I think it would make sense to just adapt the CT's code while addressing this issue, and then focus on a more complete solution over there.

I also suggested in Slack that it might make sense to split this issue, however, given that we already have one to officially introduce #multilingual, I'm wondering whether it would be fine to merge the ::hasAffectingChanges() bits into #2826021: FieldItemList::equals is sufficient from the storage perspective but not for code checking for changes, and split the rest into two separate issues: one to introduce the revision hooks and one to adapt CT's use of #multilingual.

Marked #2826021: FieldItemList::equals is sufficient from the storage perspective but not for code checking for changes as duplicate. Adding credit for hchonov and mkalkbrenner.

Crediting also Tobias for the research work :)

Hi,

I'm a little confused about the state of this issue, its patches and its related issues and their patches.

#34 states the following, but is that accurate? I don't think so.

Marked #2826021: FieldItemList::equals is sufficient from the storage perspective but not for code checking for changes as duplicate.

In #35, this issue was converted into a meta issue pointing to the other 3 issues with #36 issue summary changes and patch in related issue.

I think I've figured it out. I'm disabling the display of the files in this issue. It looks we should use the patch files found in the other issues referenced in the issue summary. Let me know if hiding the files is not preferred or I've misunderstood something here.

hiding other files

Thanks!

I committed #2975754: Add hooks to act on a new revision being created so #2826021: FieldItemList::equals is sufficient from the storage perspective but not for code checking for changes is left of this, right?

@Gábor Hojtsy, yes that is correct.