Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
While working on #2315773: Create a menu link field type/widget/formatter the following problem was observed:
- Added a content type with a translatable field
- Mark the "menu parent" column as non translatable
- Added a new content with setting the menu parent to
""
(aka. put it belowhome
) - Translate the content to the other language
- Expected behaviour: The translation can be used as expected. The original translation will be untouched.
- Actual behaviour: The original field item is dropped
Critical vs. Major
You probably don't run into that issue with core fields, even image or field item could be problematic here.
Proposed resolution
Fix the field synchronization part.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#2 | 2861367-2.patch | 2.71 KB | dawehner |
#2 | 2861367-test.patch | 1.74 KB | dawehner |
Comments
Comment #2
dawehnerHere is a test + a fix. Not sure whether this is the best approach.
Comment #3
dawehnerI'd argue this is borderline critical. I saw data being lost, even it might be an edge case.
Comment #4
dawehnerComment #7
dawehnerComment #8
dawehnerComment #9
xjmAs this is a data loss issue, let's raise this to critical for now even if it is edgecase-y.
Comment #10
catchDiscussed this on the committer triage call, two main questions:
1. As with #2766957: Forward revisions + translation UI can result in forked draft revisions I'm left asking why we allow translators to edit non-translatable fields in the first place - that's not really translation.
2. Shouldn't emptying a field remove the contents? Isn't that what happens when you aren't translating?
I've opened a new issue to discuss this, I'm not sure we can do anything here without resolving that one, so postponing for now #2878556: Ensure that changes to untranslatable fields affect only one translation in pending revisions.
Comment #11
catchComment #12
dawehnerI think its more tricky, fields are partially translatable. In my case it was the title/description of the menu being translatable, but the menu parent not. I don't think any of the decisions in #2878556: Ensure that changes to untranslatable fields affect only one translation in pending revisions would change this decision.
Let me quote the issue summary:
This is not an empty field value, its the root of a menu, and as such a total legitimate value.
Comment #13
catchOK so in that case, the form field for the menu parent should not be editable via the translation form, and the form fields for the columns that are translatable should? That still feels like the same problem as #2878556: Ensure that changes to untranslatable fields affect only one translation in pending revisions just we'd need to disable things at the form field/column level rather than/as well as at the field API field level.
Comment #14
dawehner@catch
I guess so, but even then, if you start with an empty string (root parent), go to the translation page, no matter whether the form is disabled or not, the field sync process will run and detect the empty string. Maybe i'm stubborn, but I don't see how this is not a bug in
content_translation
.Comment #15
catchSo that's interesting but I don't see how the translation sync process can distinguish between an empty string in a text field vs. an empty string in a menu parent, should the menu be storing 0 instead of ''?
Let's re-open though, sounds like it might be fixable at least partly without doing the other issue.
Comment #16
Wim LeersI suspect this is related to the use/logic of
\Drupal\Core\Field\FieldItemListInterface::equals()
.Comment #17
dawehner@Wim Leers
I think the reason why
\Drupal\Core\Field\FieldItemListInterface::equals()
might not be used here is because it needs more logic, due to the really obscure feature of translation groups, which allows to have parts of a field translatable and some others not. To be honest though, it took me a while to remotely get what was going on in this subsystem.Comment #19
xjm@cilefen, @Cottser, @effulgentsia, and I discussed this issue and agreed it is indeed critical. The tests are excellent and demonstrate the data loss problem pretty clearly. Thanks!
Comment #22
plachI'll have a look at this soonish.
Comment #29
stephencamilo CreditAttribution: stephencamilo as a volunteer commentedComment #30
matsbla CreditAttribution: matsbla commentedComment #32
xjm