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
@effulgentsia said in #2940204-22: Translatable fields with synchronization enabled should behave as untranslatable fields with respect to pending revisions:
I think for purposes of naming the method, "properties" is better than "columns", since the "column" terminology is a historical implementation artifact, not something we need to leak to the API.
Proposed resolution
Rename the $columns
parameter to $properties
for the following methods:
FieldTranslationSynchronizerInterface::synchronizeItems()
FieldTranslationSynchronizer::synchronizeItems()
FieldTranslationSynchronizer::itemHash()
(bonus :)
Remaining tasks
- Write a patch
- Reviews
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#34 | interdiff-2941058-30-34.txt | 712 bytes | bn_code |
#34 | switch-to-property-2941058-34.patch | 5.3 KB | bn_code |
Comments
Comment #2
bn_code CreditAttribution: bn_code commentedI am working on this issue. I only have free time to do so over the next couple of days.
Comment #3
plachAwesome, could you please assign the issue to yourself while working on it?
Comment #4
bn_code CreditAttribution: bn_code commentedComment #5
bn_code CreditAttribution: bn_code commentedI have switched from "column" to "property" terminology.
Comment #6
bn_code CreditAttribution: bn_code commentedComment #7
bn_code CreditAttribution: bn_code commentedFix column in a "foreach" loop.
Comment #8
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented@bn_code,
Patch attached for comment #7 applies cleanly. The fix using 'foreach' seems to be a better approach though.
Attaching interdiff below.
Comment #9
plach@bn_code:
The patch looks almost perfect, thank you!
Let's use "properties" also in
$item_columns_to_sync
and$item_columns_to_keep
, please :)@chiranjeeb2410:
Thanks for the interdiff and for the confirmation that the patch applies, however there's no need to bother testing that because it would have failed automated testing if it didn't.
Comment #10
plachComment #11
bn_code CreditAttribution: bn_code commented@plach:
Thank, you! I have changed $item_columns_to_sync and $item_columns_to_keep.
Comment #13
bn_code CreditAttribution: bn_code commentedComment #14
bn_code CreditAttribution: bn_code commentedComment #15
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented@bn_code, though the changes are known as of now for the latest patch, still adding an interdiff makes
it pretty easier to review. Recommended to add one.
Comment #16
bn_code CreditAttribution: bn_code commented@chiranjeeb2410
Thank you for your comment.
It's my first fix.
That's good to know:)
Comment #17
bn_code CreditAttribution: bn_code commentedComment #18
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commented@bn_code,
Patch serves the purpose as suggested in #9. RTBC good to go.
Comment #20
plachTestbot issue
Comment #22
chiranjeeb2410 CreditAttribution: chiranjeeb2410 at Google Summer of Code commentedTestbot has some serious issues lol
Comment #23
Gábor HojtsyShould variable comments be updated as well? I would think so.
Also is this really the only place columns are used? Eg. the annotation also uses columns to described what to sync by default.
Finally is property technically correct? We are referring to internal parts of fields (eg. the alt text of an image field).
Comment #24
BerdirYes, those are called properties. \Drupal\image\Plugin\Field\FieldType\ImageItem::propertyDefinitions(). columns is what schema() uses, but that's an internal and old key.
Comment #25
Gábor HojtsySo in this annotation, column_groups should be / should have been property_groups with this harmonization?
Comment #26
BerdirShould, yes, except changing that now is obviously not backwards compatible. So if we do that, we need to have an alter hook that standardizes on the new key.. considering that others might be adding/changing them also in an alter hook :)
Comment #27
plachYes, the correct annotation keys would be
property_groups
/properties
. But I guess deprecate those and support the new ones is out of scope for this issue.Comment #28
Gábor HojtsyWell, I read the title Switch from "column" to "property" terminology in the field translation synchronizer and was thinking what would that mean :) Is the synchroniser currently self-consistent with its terminology or not really? Because if we try to change parts of it, will make it (more) inconsistent.
Comment #29
plachIf we do this, the only inconsistent part will be those annotation keys. Right now it's a mix :)
Comment #30
bn_code CreditAttribution: bn_code commentedI've made improvements. Please review:)
Comment #31
bn_code CreditAttribution: bn_code commentedComment #33
plachThanks, but these changes need to be reverted otherwise existing code will stop working. If we want to do this, we need to keep supporting the old keys to preserve backwards-compatibility. I think it would be better to address that in a follow-up issue.
Comment #34
bn_code CreditAttribution: bn_code at EPAM Systems commented-
Comment #35
bn_code CreditAttribution: bn_code at EPAM Systems commentedOk, I have returned it.
Comment #36
plachComment #37
plachLooks good, thanks!
Comment #39
catchCommitted 7f68a3d and pushed to 8.6.x. Thanks!
Opened #2948660: Use 'property' instead of 'column' in field type annotations for the follow-up.