Problem/Motivation

The Content Translation module offers two ways to handle multilingual entities:

  • Editors, that is users with edit permissions over the entity, will be offered the regular entity form, where untranslatable fields are highlighted regardless of the language being edited (see also #2290101: UI telling you a field is shared across languages is way too subtle). This mode retains the ability to perform contextual editing: regardless of language, the editor can edit all the fields that are displayed in the entity page.
  • Translators, users with translation permissions but not edit permissions over the entity, will be offered translation forms, that is stripped-down entity forms with no widgets for untranslatable fields and other monolingual form elements. In this case contextual editing does not matter because translators need to always visit the translation overview page and only then they can access a translation form.

However, for editors the following behaviour is possible:

  1. Edit a German translation, including a change to an non-translated entity reference creating a new default revision.
  2. Revert that revision, the translated fields will revert, but the entity reference will not.

Even if we changed the behaviour in #2 to include the non-translated entity reference in the fields to revert, it's not clear if that would be correct either - the person doing the revert can't necessarily tell that the translator changed the untranslated field when they made the new revision.

This is just an example of a deeper issue: when saving a new revision having changes to untranslatable fields, all available languages are marked as affected, since there is no way to tell whether the user meant to apply the change only to a particular translation.

This is problematic for pending revisions on multilingual entities because, as pointed out in #2766957-73: Forward revisions + translation UI can result in forked draft revisions, a pending revisions should have only one affected language to be handled properly and be safely merged with the default revision when "publishing" it.

Proposed resolution

Add an option to choose between the two following behaviors: a change to an untranslatable field either affects all translations or the default translation only.

The first mode is the default core behavior, that suits well for simple workflows where revisions are not involved and contextual editing is important.

In the alternative mode, the Content Translation module will hide widgets for untranslatable fields in non-default language entity forms. In this case the edit form for non-default languages is identical to a translation form (see the attached screenshot). This may be slightly confusing, as contextual editing for untranslatable fields is no longer preserved, but more confusing issues are avoided.

An option in the CT's administrative UI is offered to toggle between the two modes per bundle.

Remaining tasks

User interface changes

API changes

Only additions: ContentEntityInterface:: isDefaultTranslationAffectedOnly().

Data model changes

None

CommentFileSizeAuthor
#72 et-untranslatable_fields_hide-2878556-71.interdiff.txt1.55 KBplach
#72 et-untranslatable_fields_hide-2878556-71.patch48.07 KBplach
#64 et-untranslatable_fields_hide-2878556-64.interdiff.txt2.4 KBplach
#64 et-untranslatable_fields_hide-2878556-64.patch48.07 KBplach
#58 et-untranslatable_fields_hide-2878556-57.interdiff.txt1.42 KBplach
#58 et-untranslatable_fields_hide-2878556-57.patch47.81 KBplach
#56 et-untranslatable_fields_hide-2878556-56.patch47.73 KBplach
#52 et-untranslatable_fields_hide-2878556-51.interdiff.txt9.01 KBplach
#51 et-untranslatable_fields_hide-2878556-51.review.patch47.73 KBplach
#51 et-untranslatable_fields_hide-2878556-51.patch75.27 KBplach
#49 et-untranslatable_fields_hide-2878556-50.interdiff.txt10.63 KBplach
#49 et-untranslatable_fields_hide-2878556-50.review.patch47.53 KBplach
#49 et-untranslatable_fields_hide-2878556-50.patch75.08 KBplach
#44 Edit_Article_Test_1_IT__Italian_translation____Drupal_8_x.png283.34 KBplach
#43 et-untranslatable_fields_hide-2878556-43.review.patch46.45 KBplach
#43 et-untranslatable_fields_hide-2878556-43.interdiff.txt2.21 KBplach
#43 et-untranslatable_fields_hide-2878556-43.patch72.46 KBplach
#41 et-untranslatable_fields_hide-2878556-41.interdiff.txt4.9 KBplach
#41 et-untranslatable_fields_hide-2878556-41.review.patch46.26 KBplach
#41 et-untranslatable_fields_hide-2878556-41.patch84.98 KBplach
#40 Edit_Article_Test_1_3___Drupal_8_x_and_d8_—_bash.png260.11 KBplach
#39 Create_Italian_translation_of_Test_1_3___Drupal_8_x_and_d8_—_bash.png295.41 KBplach
#37 et-untranslatable_fields_hide-2878556-37.review.patch42.83 KBplach
#37 et-untranslatable_fields_hide-2878556-37.interdiff.txt25.66 KBplach
#37 et-untranslatable_fields_hide-2878556-37.patch83.8 KBplach
#33 et-untranslatable_fields_hide-2878556-33.review.patch34.15 KBplach
#33 et-untranslatable_fields_hide-2878556-33.patch73.52 KBplach
#21 et-untranslatable_fields_hide-2878556-20.interdiff.txt5.69 KBplach
#20 et-untranslatable_fields_hide-2878556-20.interdiff.txt5.69 KBplach
#20 et-untranslatable_fields_hide-2878556-20.review.patch33.8 KBplach
#20 et-untranslatable_fields_hide-2878556-20.patch50.14 KBplach
#18 et-untranslatable_fields_hide-2878556-18.review.patch28.12 KBplach
#18 et-untranslatable_fields_hide-2878556-18.interdiff.txt8.77 KBplach
#18 et-untranslatable_fields_hide-2878556-18.patch44.46 KBplach
#16 untranslatable_image_file_in_translatable_in_field.png46.96 KBmatsbla
#16 language_settings_content_type.png76.67 KBmatsbla
#14 Are_you_sure_you_want_to_revert_English_translation_to_the_revision_from_Tue__11_07_2017_-_18_36____Drupal_8_x.png108.11 KBplach
#13 Edit_Moderated_Test_3_9_EN___Drupal_8_x.png257.25 KBplach
#13 Edit_Moderated_Test_3_2_IT__Italian_translation____Drupal_8_x.png164.12 KBplach
#13 Content_language___Drupal_8_x.png129.46 KBplach
#13 et-untranslatable_fields_hide-2878556-13.patch19.54 KBplach
#3 2878556-3.png185.72 KBvijaycs85
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

catch created an issue. See original summary.

matsbla’s picture

Issue summary: View changes
vijaycs85’s picture

FileSize
185.72 KB

Currently this is represented by label (with "(all languages)" suffix). Probably we can have the fields as value (markup) by default and option to allow edit?

plach’s picture

Discussed this with @catch, this is a summary of what I told him:

[This is what is currently implemented in core:] we can either have editors, who can edit the entity in every language and have access to untranslatable fields, or translators, who have access only to a stripped-down version of the entity form, presenting only translatable fields.
The reasons was that some people (including me :) found it confusing to lose contextual editing for untranslatable fields just because the current language is not the entity default one. Translators OTOH don’t have the Edit tab and need to reach the entity form via the Translate tab, which eliminates the contextual editing issue. At that point displaying only translatable field widgets is safe from a UX perspective [and is desired since translators don't have access to untranslatable fields].

I think the fact that @catch felt the need to open this means #2290101: UI telling you a field is shared across languages is way too subtle is still a very relevant issue.

To ensure we don't have an architectural flaw here, we quickly went over the issues linked in the IS:

I would be open to adding an option to hide untranslatable fields on non-default-language edit forms, but that wouldn’t solve those issues :)

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

hchonov’s picture

I think that the following use case is worth mentioning and have to be considered if some behaviour will be changed, because it is currently possible and I think it should remain possible:

  • assume that there is a non-translatable entity reference inline field (not a specific module, think just about inline editing of referenced entities)
  • no matter if the edit form of the parent entity is retrieved in a default or a non-default translation language of the entity the inline entity forms of the referenced entities are retrieved in that same translation language
  • this allows for having a non-translatable field, but editing the inline referenced entities in the current form language
amateescu’s picture

Re @plach in #4:

I would be open to adding an option to hide untranslatable fields on non-default-language edit forms, but that wouldn’t solve those issues :)

In D7, we had a "read only" field widget available to all field types in the Field extra widgets module. Quoting from the project page, its behavior was something like this:

this widget shows the content of the field on the edit form, but doesn't allow the user to edit it. The content is rendered using one of the formatters of the field.

Would this kind of widget be a possible solution for this issue?

catch’s picture

Title: Define desired behaviour for un-translatable fields. » Revision (especially revert) behaviour and untranslatable fields is not clear
Issue summary: View changes
Issue tags: +Workflow Initiative
catch’s picture

Assigned: catch » Unassigned
plach’s picture

Assigned: Unassigned » plach

I'm doing some work on this subject as part of #2860097: Ensure that content translations can be moderated independently. I will post an initial patch implementing the following (optional) behavior ASAP:

Can we remove the ability to edit shared fields apart from in the original language of entities?

plach’s picture

Here is a patch to add the option to choose between the two following behaviors: a change to an untranslatable field either affects all translations or the default translation only.

If the latter mode is enabled the Content Translation module will hide widgets for untranslatable fields. In this case the edit form for non-default languages is identical to a translation form (see the attached screenshot).

An option in CT's administrative UI is offered to toggle between the two modes per bundle (see the attached screenshot).

Tagging for usability review. Aside from that this needs test coverage as what I implemented so far belongs in #2860097: Ensure that content translations can be moderated independently.

plach’s picture

plach’s picture

Assigned: plach » Unassigned
matsbla’s picture

I made a quick check, this is a nice feature!

Just 2 comments;
maybe the check box also should be available under the language setting for each content type?

And also I tried with a translatable image field having an untranslatable image file, however I'm then still able to edit the image file on the translation form.

plach’s picture

@matsbla:

Good points!

maybe the check box also should be available under the language setting for each content type?

Ideally yes, however I'd like to avoid adding too many changes here and I'm slightly concerned by the need to always replicate UI items on the content type configuration pages. What about opening a new issue to find a standardized way to reuse these UI components both in the Language/Content Translation settings and in the Content types configuration pages?

And also I tried with a translatable image field having an untranslatable image file, however I'm then still able to edit the image file on the translation form.

Mmh, this is tricky to address because synchronization implies a translatable field. We should come up with some custom logic to deal with this. I'll mull on it a bit more. Maybe a follow-up would make sense here too?

plach’s picture

Title: Revision (especially revert) behaviour and untranslatable fields is not clear » [PP-1] Revision (especially revert) behaviour and untranslatable fields is not clear
Related issues: +#2924724: Add an API to create a new revision correctly handling multilingual pending revisions
FileSize
44.46 KB
8.77 KB
28.12 KB

To provide proper test coverage, this now relies on the kernel test introduced in #2924724: Add an API to create a new revision correctly handling multilingual pending revisions. The attached patch adds test coverage for the API bits. We still need test coverage for the Content Translation bits. We also need feedback about #17.

plach’s picture

Assigned: Unassigned » plach

Working on the additional test coverage.

plach’s picture

Here's the additional test coverage. Attaching the combined patch and the one to review.

plach’s picture

Correct interdiff

plach’s picture

Assigned: plach » Unassigned
Gábor Hojtsy’s picture

@plach asked to review. This is a bit of a usability review (but not a complete one) and a bit of a code review (but absolutely not a complete one).

  1. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraint.php
    @@ -0,0 +1,20 @@
    +  public $message = 'Fields shared among translations can be changed only in default revisions.';
    

    Is this true? The issues proposes that they only be editable in default TRANSLATIONS (AKA original language) not default REVISIONS?

  2. +++ b/core/modules/content_translation/config/schema/content_translation.schema.yml
    @@ -18,3 +18,17 @@ language.content_settings.*.*.third_party.content_translation:
    +content_translation.settings:
    +  type: config_object
    +  label: 'Content translation settings'
    +  mapping:
    +    untranslatable_fields_hide:
    +      type: sequence
    +      label: 'Entity types'
    +      sequence:
    +        type: sequence
    +        label: 'Bundles'
    +        sequence:
    +          type: boolean
    +          label: 'Hide shared fields on translation forms'
    

    Existing config structure is not extendable to store this? It looks odd to introduce yet another config place to store this. It makes shipping settings harder for example in modules/distros if this file is global site-wide.

  3. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -110,6 +112,22 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    +        '#title' => t('Hide shared fields on translation forms'),
    +        '#description' => t('This will prevent editors from changing field values shared among translations when editing content translations.'),
    

    Is this called a shared field elsewhere on the UI? I would drop the explanation. I jump first on saying "This will" is unnecessary but on second look the whole description is superfluous.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Also the title says its postponed but not explained on what...

hchonov’s picture

I've just talked with @plach about this issue and I think that it would be nice if we show a notice on the "field add form" for untranslatable fields e.g. "If you define the field to be non-translatable then it can be only edited on the entity default translation, because the entity is configured this way.".

hass’s picture

I‘m wondering how this should work.

In a normal website every language has it‘s own edit permission. Not every translator should be able to edit every language because of language skills.

In such a case a user may has no permission on the default language.

plach’s picture

And for the same reasons in the scenario you are describing translators should not be able to edit untranslatable fields. In fact Content Translation is already exposing translation forms allowing to edit only translatable fields for users having translation permissions but not edit permissions over the entity.

Gábor Hojtsy’s picture

Issue tags: -Needs usability review

We reviewed this UI proposal on the UX meeting today. Short summary: hiding the fields looks sensible. There should be some indication though that the message was hidden. As per @yoroy that would be an informational AKA blue message. We did not discuss but there is also #23/3 that I still stand by.

plach’s picture

Priority: Critical » Major
Issue tags: +Triaged D8 major

@catch, @effulgentsia, @larowlan, @xjm and I discussed this issue today. We agreed to downgrade it to major as this is enabling great progress, but there is no critical functionality strictly blocked on this.

plach’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated the IS.

plach’s picture

Title: [PP-1] Revision (especially revert) behaviour and untranslatable fields is not clear » [PP-1] Ensure that untranslatable fields play well with pending revisions
plach’s picture

Issue summary: View changes

Minor clean-up

plach’s picture

Title: [PP-1] Ensure that untranslatable fields play well with pending revisions » [PP-2] Ensure that untranslatable fields play well with pending revisions
FileSize
73.52 KB
34.15 KB
hass’s picture

@plach:

And for the same reasons in the scenario you are describing translators should not be able to edit untranslatable fields.

I‘m confused. If an images field is not transatable what makes often sense every translator need to be able to add additional images. But with this logic this becomes impossible. Should I always ask a different translator to add an image for me? I think not.

plach’s picture

Title: [PP-2] Ensure that untranslatable fields play well with pending revisions » [PP-1] Ensure that untranslatable fields play well with pending revisions
Issue summary: View changes

Updated IS

plach’s picture

Issue summary: View changes
plach’s picture

This should implement Gabor's suggestions. The only problem I encountered is that we don't have an "info" message type, so for now I set it as a warning.

plach’s picture

Status: Needs work » Needs review
plach’s picture

Here's an updated screenshot

plach’s picture

One more screenshot

plach’s picture

Fixed change detection logic for untranslatable fields in pending revisions. I think this should be ready for review now.

Gábor Hojtsy’s picture

Just a quick review of the UI update since I don't think I am qualified to review the deeper logic:

+++ b/core/modules/content_translation/src/ContentTranslationHandler.php
@@ -536,6 +576,11 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
+      $this->messenger->addWarning($this->t('All fields that apply to all languages were hidden to avoid conflicting changes. You can edit them <a href="@url">on the original language</a> form.', ['@url' => $url]));

I would shorten this to.:

Fields that apply to all languages are hidden to avoid conflicting changes. Edit them on the original language form.

("All fields" gets confusing with "all languages" + "You can" is superfluous while "edit is the action and should be part of the link because it leads to editing).

plach’s picture

Updated screenshot.

The last submitted patch, 43: et-untranslatable_fields_hide-2878556-43.patch, failed testing. View results

gabesullice’s picture

Status: Needs review » Needs work

This review is purely for code. I haven't looked at the UI.

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -1406,10 +1400,15 @@ public function hasTranslationChanges() {
    +    // We check also untranslatable fields, so that a change to those will mark
    

    We also check

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
    @@ -41,4 +41,13 @@ public function getLoadedRevisionId();
    +  public function isDefaultTranslationAffectedOnly();
    

    Good method naming :)

  3. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -217,13 +217,13 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
    +      // By default we copy untranslatable field values from the default
    +      // revision, unless they are configured to affect only the default
    +      // translation. This way we can ensure we always have only one affected
    +      // translation in pending revisions. This constraint is enforced by
    +      // EntityUntranslatableFieldsConstraintValidator.
    

    This is an excellent comment.

  4. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraint.php
    @@ -0,0 +1,20 @@
    +  public $message = 'Non translatable fields can only be changed either when updating the current revision or the original language.';
    

    s/either//g

  5. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraintValidator.php
    @@ -0,0 +1,96 @@
    +    // Untranslatable fields restrictions apply only to pending revisions of
    

    s/fields/field/g

  6. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraintValidator.php
    @@ -0,0 +1,96 @@
    +    // To avoid unintentional reverts and data losses, we forbid changes to
    +    // untranslatable fields in pending revisions for multilingual entities. The
    +    // only case where changes in pending revisions are acceptable is when
    +    // untranslatable fields affect only the default translation, in which case
    +    // a pending revision contains only one affected translation. Even in this
    +    // case, multiple translations would be affected in a single revision, if we
    +    // allowed changes to untranslatable fields while editing non-default
    +    // translations, so that is forbidden too.
    

    Another excellent comment.

  7. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraintValidator.php
    @@ -0,0 +1,96 @@
    +   * Checks whether an entity has untranslatable fields changes.
    

    s/fields/field/g

  8. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraintValidator.php
    @@ -0,0 +1,96 @@
    +      \Drupal::entityTypeManager()
    +        ->getStorage($entity->getEntityTypeId())
    +        ->loadRevision($entity->getLoadedRevisionId());
    

    The entity type manager can be injected by implementing the Drupal\Core\DependencyInjection\ContainerInjectionInterface.

    See ValidReferenceConstraintValidator for an example.

  9. +++ b/core/modules/content_translation/config/schema/content_translation.schema.yml
    @@ -18,3 +18,9 @@ language.content_settings.*.*.third_party.content_translation:
    +    bundle_settings:
    +      type: sequence
    +      label: 'Content translation bundle settings'
    +      sequence:
    +        type: string
    +        label: 'Bundle settings values'
    

    I'm not sure if this needs an update hook or not, but raising it as a concern to be double-checked.

  10. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -110,6 +112,23 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    +          '#default_value' => !empty($settings['untranslatable_fields_hide']),
    

    Do we need an isset check here too?

  11. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -317,6 +336,8 @@ function content_translation_form_language_content_settings_validate(array $form
    +  $manager = \Drupal::service('content_translation.manager');
    

    Let's name this $content_translation_manager

  12. +++ b/core/modules/content_translation/content_translation.module
    @@ -161,9 +162,15 @@ function content_translation_entity_type_alter(array &$entity_types) {
    +  $manager = \Drupal::service('content_translation.manager');
    

    Same thing.

  13. +++ b/core/modules/content_translation/content_translation.module
    @@ -161,9 +162,15 @@ function content_translation_entity_type_alter(array &$entity_types) {
    +        $bundle_info['untranslatable_fields.default_translation_affected'] = !empty($settings['untranslatable_fields_hide']);
    

    Same concern about this not being set.

  14. +++ b/core/modules/content_translation/src/BundleSettingsInterface.php
    @@ -0,0 +1,35 @@
    +namespace Drupal\content_translation;
    

    Would Drupal\Core\Entity be a better namespace for this?

  15. +++ b/core/modules/content_translation/src/BundleSettingsInterface.php
    @@ -0,0 +1,35 @@
    + * Interface providing support for content translation bundle settings.
    + */
    +interface BundleSettingsInterface {
    

    If not, can we name this BundleTranslationSettingsInterface? Same for the methods.

  16. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
    @@ -83,14 +96,17 @@ class ContentTranslationHandler implements ContentTranslationHandlerInterface, E
    +   * @param \Drupal\Core\Messenger\MessengerInterface
    

    Missing variable name.

  17. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
    @@ -102,7 +118,8 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
    +      \Drupal::messenger()
    

    The messenger isn't in the container??

  18. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
    @@ -512,6 +531,20 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
    +    // hidden. Fields that are not involved in translation changes checks, for
    +    // instance the "revision_log" field, should not be affected by this logic.
    

    "Field that are not involved in translation changes checks should not be affected by this logic (the "revision_log" field, for instance)."

  19. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
    @@ -512,6 +531,20 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
    +    $field_definitions = array_diff_key($entity->getFieldDefinitions(), array_flip($this->getFieldsToSkipFromTranslationChangesCheck($entity)));
    

    $untranslatable_field_definitions

  20. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
    @@ -536,6 +576,11 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
    +      $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]));
    

    Good message :)

    Should we s/@url/:url/g?

  21. +++ b/core/modules/content_translation/src/ContentTranslationManager.php
    @@ -8,7 +8,7 @@
    +class ContentTranslationManager implements ContentTranslationManagerInterface, BundleSettingsInterface {
    

    Ah, I see. This should definitely be BundleTranslationSettingsInterface.

  22. +++ b/core/modules/content_translation/src/ContentTranslationManager.php
    @@ -105,6 +105,23 @@ public function isEnabled($entity_type_id, $bundle = NULL) {
    +  public function setBundleSettings($entity_type_id, $bundle, array $settings) {
    ...
    +  public function getBundleSettings($entity_type_id, $bundle) {
    

    Same for these methods.


Wow... this was obviously took a tremendous amount of effort and intricacy. Fantastic job. 👏

xjm’s picture

Can we update this issue with a more specific title? It's very easy to get confused about which issue is which in this series.

plach’s picture

Title: [PP-1] Ensure that untranslatable fields play well with pending revisions » [PP-1] Ensure that untranslatable fields affect only one translation in pending revisions
plach’s picture

Wow, this was a big one, thanks! :)

1: Fixed
4: Fixed
5: Fixed
7: Fixed
8: Fixed
9: I'm not 100% sure, but I think we should be fine, since we default to FALSE and missing configuration values are interpreted as FALSE.
10/13: empty($a['k']) is equivalent to !isset($a['k']) || !$a['k'], in fact "empty() does not generate a warning if the variable does not exist." (php.net).
11: Fixed
12: Fixed
14: I don't think so, this is a CT-specific interface. Actually the only reason it exists is to preserve BC, since adding methods to ContentTranslationManagerInterface would be a BC break, give it has no corresponding base class.
15/21/22: This interface lives in the CT namespace, I think adding a translation prefix would be redundant.
16: Fixed
17: Nope, see https://api.drupal.org/api/drupal/core%21lib%21Drupal.php/function/Drupa...
18: Fixed
19: Those are not untranslatable field definitions, those are all the definitions being processed. Untranslatable fields are identified in the loop body.
20: Good catch!

gabesullice’s picture

Status: Needs review » Needs work
  1. I'm not 100% sure, but I think we should be fine, since we default to FALSE and missing configuration values are interpreted as FALSE.

    Cool.

  2. empty($a['k']) is equivalent to !isset($a['k']) || !$a['k'], in fact "empty() does not generate a warning if the variable does not exist." (php.net).

    Mind blown. How have I not known this for so long?! Thanks ;)

  3. This interface lives in the CT namespace, I think adding a translation prefix would be redundant.

    Not a blocker for me, so I'll leave it up to you. I made my suggestion because all of these already exist:

    Drupal\content_translation\ContentTranslationHandlerInterface
    Drupal\content_translation\ContentTranslationManagerInterface
    Drupal\content_translation\ContentTranslationMetadataWrapperInterface
    Drupal\content_translation\FieldTranslationSynchronizerInterface
    
  4. +++ b/core/modules/content_translation/src/ContentTranslationHandler.php
    @@ -541,8 +541,8 @@ public function entityFormSharedElements($element, FormStateInterface $form_stat
    +    // hidden. Field that are not involved in translation changes checks should
    

    s/Field/Fields/g. I'm sorry that was my mistake.

  5. Nope, see https://api.drupal.org/api/drupal/core%21lib%21Drupal.php/function/Drupa...

    That's a confusing set of issues and change record... but there is a service. I checked with the authors of those patches and they recommended $container->get('messenger'); and pointed me to #2924538: [META] Remove all usages of drupal_get_message and drupal_set_message where you can see where that exact pattern is used.


That was a huge initial review, so thanks for bearing with me :)

plach’s picture

3: Fair enough, fixed
4: Fixed
5: I totally missed that, thanks! Fixed

plach’s picture

Forgot the interdiff

plach’s picture

Issue summary: View changes
gabesullice’s picture

Status: Needs review » Reviewed & tested by the community

Awesome work!

plach’s picture

Status: Reviewed & tested by the community » Postponed

Thanks, let's wait for the dependency to be committed :)

plach’s picture

plach’s picture

Title: [PP-1] Ensure that untranslatable fields affect only one translation in pending revisions » Ensure that untranslatable fields affect only one translation in pending revisions
plach’s picture

gabesullice’s picture

Status: Needs review » Reviewed & tested by the community

Minor interface clean-up.

Good catch.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

Looks pretty good, a few minor questions:

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
    @@ -262,6 +262,11 @@ public function createRevision(RevisionableInterface $entity, $default = TRUE, $
    +
    +      // Populate the "original" property with the current values, given that
    +      // the new revision is not stored anywhere. This way we can detect changes
    +      // properly.
    +      $new_revision->original = clone $new_revision;
    

    This could possibly use some more explanation as to why we're mocking $entity->original.

  2. +++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraintValidator.php
    @@ -0,0 +1,125 @@
    +    /** @var \Drupal\Core\Entity\ContentEntityInterface $original */
    +    $original = isset($entity->original) ?
    +      $entity->original :
    +      $this->entityTypeManager
    +        ->getStorage($entity->getEntityTypeId())
    +        ->loadRevision($entity->getLoadedRevisionId());
    +
    

    Isn't this ternary more lines than an if/else would be?

  3. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -110,6 +112,23 @@ function _content_translation_form_language_content_settings_form_alter(array &$
     
    +      // Displayed the "shared fields widgets" toggle.
    +      if ($content_translation_manager instanceof BundleTranslationSettingsInterface) {
    +        $settings = $content_translation_manager->getBundleTranslationSettings($entity_type_id, $bundle);
    +        $form['settings'][$entity_type_id][$bundle]['settings']['content_translation']['untranslatable_fields_hide'] = [
    +          '#type' => 'checkbox',
    +          '#title' => t('Hide non translatable fields on translation forms'),
    +          '#default_value' => !empty($settings['untranslatable_fields_hide']),
    +          '#states' => [
    +            'visible' => [
    +              ':input[name="settings[' . $entity_type_id . '][' . $bundle . '][translatable]"]' => [
    +                'checked' => TRUE,
    +              ],
    +            ],
    +          ],
    +        ];
    +      }
    

    How much of this is because we want to make this behaviour configurable, vs making it configurable to provide continuity for existing sites?

effulgentsia’s picture

How much of this is because we want to make this behaviour configurable, vs making it configurable to provide continuity for existing sites?

From the issue summary:

The first mode is the default core behavior, that suits well for simple workflows where revisions are not involved and contextual editing is important.

So I think it makes sense to allow for the first mode on sites that don't have pending revisions.

However, rather than making it configuration, I wonder if we should make content_moderation force the second mode (hide untranslatable fields from entity edit forms of non-default translations). Via an API that contrib modules that allow for pending revisions other than Content Moderation could also invoke.

plach’s picture

However, rather than making it configuration, I wonder if we should make content_moderation force the second mode (hide untranslatable fields from entity edit forms of non-default translations). Via an API that contrib modules that allow for pending revisions other than Content Moderation could also invoke.

Since the new mode can be useful also to sites using only default revisions that are experiencing troubles with reverts, I think an explicit option would ensure more flexibility. Additionally there's a similar option in Entity Translation, so people upgrading from D7 will likely be already familiar with that.

plach’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 64: et-untranslatable_fields_hide-2878556-64.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

plach’s picture

Status: Needs work » Reviewed & tested by the community

Bot fluke

effulgentsia’s picture

I discussed #63 with @plach, and he convinced me that the answer to #61.3 is that it does make sense as something that's independently configurable on its own merits, not merely for continuity of existing sites. I'll try to write up why later today, to see if I understand the reasoning correctly.

plach’s picture

Title: Ensure that untranslatable fields affect only one translation in pending revisions » Ensure that changes to untranslatable fields affect only one translation in pending revisions
plach’s picture

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

@plach: let's not self-RTBC your updates after reviews. I reviewed the update you posted and it looked fine.

This has been reviewed by the UX team on the regular UX meetings, discussed with subsystem maintainers, reviewed by various core committers. As for the latest question that got settled, I agree having a setting is best for backwards compatibility and for other translation scenarios. It makes our translation experience more flexible. It does require sites to set this up explicitly the way they need to. I think we may be able to improve on that in the future while keeping the flexible configurability.

There are two minor code style problems found while commit:

FILE: ...s/content_translation/src/BundleTranslationSettingsInterface.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 28 | ERROR | Doc comment for parameter $entity_type_id does not
    |       | match actual variable name <undefined>
----------------------------------------------------------------------

FILE: ...ts/src/Functional/ContentTranslationUntranslatableFieldsTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 118 | ERROR | [x] The closing brace for the class must have an empty
     |       |     line before it
----------------------------------------------------------------------
plach’s picture

Addressed #71.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good thanks!

  • Gábor Hojtsy committed 27c3b40 on 8.6.x
    Issue #2878556 by plach, matsbla, vijaycs85, Gábor Hojtsy, catch,...

  • Gábor Hojtsy committed d229279 on 8.5.x
    Issue #2878556 by plach, matsbla, vijaycs85, Gábor Hojtsy, catch,...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for fixing these last issues so fast. Committed 27c3b40 and pushed to 8.6.x, cherry-picked to 8.5.x.

Gábor Hojtsy’s picture

Published the two change record drafts.

hass’s picture

I‘m need to say I not a fan of hiding a field. Disablng is much better! Otherwise I will get asked where the field is if soneone believe he need to change it. Showing disabled will at least show it and a description can tell the user why it is locked. That is much better UX.

effulgentsia’s picture

@hass: can you open a followup issue for that?

Wim Leers’s picture

This was committed without @effulgentsia's write-up mentioned in #67.

effulgentsia’s picture

That's ok. No need to hold up progress on my account. I'll still write up that comment when I have a chance.

Gábor Hojtsy’s picture

@hass we added this message with that thinking as per the issue summary:

There are two problems at least with hidden fields:

  • The light gray affordance used to depict hidden fields in most browsers is often not clear enough and people with vision problems will click into them and get frustrated.
  • When items are in fieldsets, etc. we would disable the underlying fields but people would still go to the trouble of trying to find them only to find them disabled.
hass’s picture

This are already UX problems for people without vision problems... it cannot become better for people with vision problems than :-). Every few weeks I already have this type of discussions in D7 just because something is temporary hidden and they do not know why or it shows as admin, but not for non-admins. I think we just need to put a strong red note that this need to edited at foo. Something people with vision problems can also read and they do not get frustrated. Hopefully.

hchonov’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1447,4 +1446,14 @@ public function hasTranslationChanges() {
+    return !empty($bundle_info[$bundle_name]['untranslatable_fields.default_translation_affected']);

+++ b/core/lib/Drupal/Core/Entity/Plugin/Validation/Constraint/EntityUntranslatableFieldsConstraintValidator.php
@@ -0,0 +1,128 @@
+    if ($this->hasUntranslatableFieldsChanges($entity)) {
+      if ($entity->isDefaultTranslationAffectedOnly()) {

+++ b/core/modules/content_translation/content_translation.module
@@ -161,9 +162,15 @@ function content_translation_entity_type_alter(array &$entity_types) {
+        $bundle_info['untranslatable_fields.default_translation_affected'] = !empty($settings['untranslatable_fields_hide']);

So it looks like we have one more place where forward revisions behavior defined by content moderation has made it's way into core. So now if I've enabled the setting "untranslatable_fields_hide" I'll get the content moderation behavior regarding forward revisions even if the content moderation module isn't enabled? If this is the case then the change in content_translation_entity_type_alter() should be reverted.

effulgentsia’s picture

@hchonov: I really appreciate you looking at this problem space from the perspective of potential alternate (non-CM) use cases of pending revisions. I haven't caught up with the discussion in #2940575: Document the scope and purpose of pending revisions yet, but look forward to doing so soon.

So now if I've enabled the setting "untranslatable_fields_hide" I'll get the content moderation behavior regarding pending revisions even if the content moderation module isn't enabled?

What's the Content Moderation behavior that you'll be getting? AFAICT, all you'll get is the ability to make changes to untranslatable fields when editing the entity in its default language, regardless of whether it's in a pending revision or not. But you'll get a validation error if you try to make those edits in the non-default language (which seems fairly consistent with the fact that those fields are hidden from the form anyway). What part of the hunks in #84 is incompatible with or not desirable for non-CM uses of pending revisions?

hchonov’s picture

What's the Content Moderation behavior that you'll be getting? AFAICT, all you'll get is the ability to make changes to untranslatable fields when editing the entity in its default language, regardless of whether it's in a pending revision or not. But you'll get a validation error if you try to make those edits in the non-default language (which seems fairly consistent with the fact that those fields are hidden from the form anyway). What part of the hunks in #84 is incompatible with or not desirable for non-CM uses of pending revisions?

@effulgentsia, I am sorry if there is something I am not understanding correctly, but to me it looks like the new constraint prevents the saving of forward revisions with multiple translations and changes in non-translatable fields. This is the CM behavior, which now gets automatically activated if the setting "untranslatable_fields_hide" is enabled. By looking at the name of the setting - 'Hide non translatable fields on translation forms' - I would simply expect that all this setting causes is that non-translatable fields are removed from translation forms and nothing else, but it additionally activates the CM behavior/constraint regarding forward revisions and prevents editing non-translatable fields in forward revisions with multiple translations.

P.S. by further looking at the constraint validator it looks like it prevents the editing of untranslatable fields in multilingual forward revisions even if the setting is not enabled. I haven't noticed the else branch until now. This is CM behavior only as well.

plach’s picture

This is the CM behavior

This is not the CM behavior, this new validator was introduced to complement the new API we introduced in #2924724: Add an API to create a new revision correctly handling multilingual pending revisions. As we discussed at length, we want to avoid data integrity issues by encouraging people to change one single translation at a time in pending revisions. This is the only approach that allows to work independently on multiple translations (and merge the result into default revisions), without needing to rely on a conflict management solution. This is especially tricky when untranslatable fields come into play, because you may end up performing this kind of problematic changes without even realizing it. Hence we decided to introduce this validator to avoid that.

Again, this logic is exploited by Content Moderation (because that's the new suggested approach), but it's not "the CM behavior" because every module out there, regardless of its interpretation of pending/forward revisions, is encouraged to adopt this logic. Of course there can be cases where you may want to implement a custom merging logic and whatnot but it's unfair to say this is CM-specific. In fact every bit of this logic lives in the core entity system.

hchonov’s picture

This is not the CM behavior, this new validator was introduced to complement the new API we introduced in #2924724: Add an API to create a new revision correctly handling multilingual pending revisions. As we discussed at length, we want to avoid data integrity issues by encouraging people to change one single translation at a time in pending revisions. This is the only approach that allows to work independently on multiple translations (and merge the result into default revisions), without needing to rely on a conflict management solution.

What exactly is the problem of using conflict management? If a user is able to further edit the default and forward revisions sooner or later there will be conflicts even in translatable fields and conflict management is necessary. Having a special behavior for forward revisions is exactly what CM needs and therefore that is the CM behavior or there is another use case which needs exactly the same behavior?

In fact every bit of this logic lives in the core entity system.

Yes and I've mentioned it already at couple of issues that this is not good, because CM and CM only is needing this API restrictions, which forces the CM behavior to be the default core one.

because every module out there, regardless of its interpretation of pending/forward revisions, is encouraged to adopt this logic.

Every module out there is forced and not encouraged. This makes a big difference.

So think about it if there is a use case like autosave using forward revisions and after the update to Drupal 8.5.0 suddenly there will be validation errors, because we suddenly force everybody to use forward revisions just like CM needs it. Therefore I would say that this change breaks BC.

plach’s picture

@hchonov:

What exactly is the problem of using conflict management?

There is nothing wrong with that. As I told you a couple of times, I think it's great and that it should be in core. But it's not, so we cannot 100% rely on it. This is why we need an alternative way to prevent the contrib/custom space from shooting themselves in the foot. If they are wearing bulletproof boots, fine, they just need to remove the validator. Once conflict management is core I'm happy to get rid of that logic, at least when conflict management is enabled.

If a user is able to further edit the default and forward revisions sooner or later there will be conflicts even in translatable fields and conflict management is necessary.

Yep, but there is nothing in core that allows you to edit multiple translations in the same pending revision, so you need some explicit code to do that. OTOH untranslatable fields let you do it without even realizing it.

Having a special behavior for forward revisions is exactly what CM needs and therefore that is the CM behavior or there is another use case which needs exactly the same behavior?

Say you have an autosave module relying on pending revisions and you have no conflict management solution enabled. You edit an article and then pause working. While you are away, someone else starts editing the same article, but in another language, changes also an untranslatable field, and saves a new default revision. Now you get back to your desk, recover your work, and save a new default revision. You've just overridden part of the other person's work without even knowing it, since you reverted the untranslatable field to its previous value.

Yes and I've mentioned it already at couple of issues that this is not good, because CM and CM only is needing this API restrictions

You have no data to support this claim, so far you only provided a couple of examples that do not need those restrictions, and they both (at least implicitly) rely on conflict management, so they need to address the same issues the validator addresses. For this statement to be true, you'd have to demonstrate that every possible use case out there does not need to rely on such a behavior, which is not true even for your own examples.

I have been saying how it's bad that we allow to change multiple translations in the same revision (even a default one) way before CM was even a thing, so I simply reject this statement.

which forces the CM behavior to be the default core one.

We are enforcing a default behavior that most people actively involved in this area deem sensible, which is what usually core does. There's nothing stopping the contrib/custom space from disabling this logic with a one line change in an alter hook.

Every module out there is forced and not encouraged. This makes a big difference.

I'd argue that at the moment two ways exist to address the data integrity issues being solved here: the validator introduced here and conflict management. If the latter takes care of unsetting the validator when it is enabled, then no other module in the contrib/custom space needs to worry about these problems: they either go the core way or they install conflict management.

Therefore I would say that this change breaks BC.

Sure. Also a bug fix breaks BC if you were relying on the buggy behavior and yet we fix bugs all the time. Not every behavior change can be considered a break of our BC policy, especially in an area that was clearly not mature enough in core, before the Workflow Initiative started improving it. Our BC policy concerns very specific items, for everything else it states that we will "take reasonable efforts to not break code that developers may be relying on". We don't guarantee you will never have to touch your code between minors, and for a good reason, if you ask me. Sometimes a small change now is preferable over having to deal with very serious issues later, because the root issue was not addressed strictly enough initially.

hchonov’s picture

If a user is able to further edit the default and forward revisions sooner or later there will be conflicts even in translatable fields and conflict management is necessary.

Yep, but there is nothing in core that allows you to edit multiple translations in the same pending revision, so you need some explicit code to do that. OTOH untranslatable fields let you do it without even realizing it.

This is not what I've meant. I've meant, that you create a draft and allow the users to edit drafts and default revisions at the same time in the same language which at the end results into a conflicts even for translatable fields. This is completely the same as if having conflicts in non-translatable fields. If content moderation doesn't currently support this and doesn't want to support this then this is no reason for it to require those significant changes, which prevent contrib from providing such a solution and yes the changes here forbid this.

Say you have an autosave module relying on pending revisions and you have no conflict management solution enabled. You edit an article and then pause working. While you are away, someone else starts editing the same article, but in another language, changes also an untranslatable field, and saves a new default revision. Now you get back to your desk, recover your work, and save a new default revision. You've just overridden part of the other person's work without even knowing it, since you reverted the untranslatable field to its previous value.

No, this is not correct. You will not override anything as without conflict management you'll not be able to save.

You have no data to support this claim, so far you only provided a couple of examples that do not need those restrictions, and they both (at least implicitly) rely on conflict management, so they need to address the same issues the validator addresses. For this statement to be true, you'd have to demonstrate that every possible use case out there does not need to rely on such a behavior, which is not true even for your own examples.

Let me ask it the other way around - could you demonstrate that every possible use case out there does need to rely on the CM behaivor? What makes you think it is not true for my own examples? I've given you an example for CM which requires conflict management as well and this is the use case where a translatable field is edited in both the default revision and in the draft one after the draft has been created.

We are enforcing a default behavior that most people actively involved in this area deem sensible, which is what usually core does.

Are those people the people working on content moderation, or are there others involved, who also work on other projects making use of forward revisions?

There's nothing stopping the contrib/custom space from disabling this logic with a one line change in an alter hook.

There is nothing stopping content moderation to enforce this behavior only for the entity types it supports instead of making this a default behavior and having to discuss about this.

Sure. Also a bug fix breaks BC if you were relying on the buggy behavior and yet we fix bugs all the time. Not every behavior change can be considered a break of our BC policy, especially in an area that was clearly not mature enough in core, before the Workflow Initiative started improving it. Our BC policy concerns very specific items, for everything else it states that we will "take reasonable efforts to not break code that developers may be relying on". We don't guarantee you will never have to touch your code between minors, and for a good reason, if you ask me. Sometimes a small change now is preferable over having to deal with very serious issues later, because the root issue was not addressed strictly enough initially.

If we don't have to break BC and have other ways of solving bugs, then we don't break BC. The issue here could have been solved without breaking BC. So the question is why? Why should we break BC when we don't have to?


Actually we've opened a new issue and agreed to decide how we want to handle forward revisions in core #2940575: Document the scope and purpose of pending revisions and the change here should be made public, if we decide to, only after the other issue is closed.

Additionally there are use cases like with the content translation outdated flag which by editing a single translation results into updating fields of the other translations as well. If I create a draft, change a non-translatable field and select the "outdated" flag to mark all other translations as outdated, then I'll have to remove all other translations in order to save and because of this later I'll have to somehow make the changes that would've been done by the form and entity API automatically. I prefer to rely on conflict management instead of having to figure out how to deal with the problems I will have to face if I remove translations and later add them again.

If I have installed the conflict module, then core will still not allow me to save multilingual forward revisions with changes in non-translatable fields, because of the constraint introduced here.

One important question - let's assume I've removed all other translations of a forward revision just to be able to create a forward revision with changes in a non-translatable field, then I've edited the non-translatable field in the default revision, and later I want to make the forward revision the default one. How do I solve the conflict now? Because even if there is only one translation in the forward revision I am still able of creating conflicts in this field. So how do these changes here help me?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.