Updated: Comment #21
Problem/Motivation
The "field synchronization" feature provided by the FieldTranslationSynchronizer class is written with the "old / BC" syntax for accessing field values. It works in HEAD because calling code takes care of passing entities wrapped in the BC layer.
#1969728: Implement Field API "field types" as TypedData Plugins rewrites a fair amount of the code flow surrounding FieldTranslationSynchronizer, and triggers hard to debug test fails related to the BC layer.
At this point in the cycle, all fieldable entity types in core implement EntityNG, aside from the 'test_entity' used in some Field API tests, that do not leverage "field synchronization", that is fully tested separately.
So, it makes more sense to move that code to work on EntityNG, which will need to happen anyway, and state that it will not work on the one entity type that is not NG yet, rather than spending time in debugging the BC layer.
Proposed resolution
Rewrite field sync on top of NG entities
#7 - #11
using item arrays in ::synchronizeFields() and setting the updated values back onto the entity afterwards,
allows keeping synchronizeItems() easily unit testable by working on plain arrays
User interface changes
No.
API changes
The "field synchronization" will not work on the 'test_entity' entities still used by some Field API tests.
Related Issues
This is blocking these:
Just Related
- #2018685: Remove field_is_translatable()
- All issues with the Entity Field API tag to show up http://entity.worldempire.ch/conversions.
- #1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget
Comment | File | Size | Author |
---|---|---|---|
#26 | minor-followup-2013837.patch | 683 bytes | yched |
#20 | field-sync-NG-2013837-20.patch | 13.43 KB | yched |
#20 | interdiff.txt | 1.86 KB | yched |
#16 | field-sync-NG-2013837-16.interdiff.do_not_test.patch | 11.77 KB | plach |
#16 | field-sync-NG-2013837-16.patch | 13.42 KB | plach |
Comments
Comment #1
yched CreditAttribution: yched commentedInitial patch - naïve, possibly/probably buggy rewrite.
Fails, but I couldn't really investigate why so far.
Comment #3
yched CreditAttribution: yched commenteder - forgot to merge HEAD before I rolled the patch
Comment #5
yched CreditAttribution: yched commentedA couple fixes, at least synchronization runs.
Still fails, fids don't match.
Comment #6
yched CreditAttribution: yched commentedSlightly better. But fids still don't match.
Comment #7
plachIt seems this is making
::synchronizeItems()
heavily depend on field NG internals: would it be possible to switch to item arrays in::synchronizeFields()
and set the updated values back onto the entity afterwards? I suspect this might make things easier.Comment #8
yched CreditAttribution: yched commented@plach: yeah, I've been hesitating to go that way.
On the plus side it would keep synchronizeItems() easily unit testable by working on plain arrays. On the minus side, it's doing back and forth translation to an old-style array format that doesn't actually exist anywhere else anymore.
Both approaches seem valid to me, your call :-)
Comment #10
plachNo strong preferences but actually this might make things more resuable, although right now I cannot imagine code needing to re-use this stuff ;)
Comment #11
yched CreditAttribution: yched commentedYou're probably right, writing the algorithm with arrays sounds right.
Can you take over from there ? :-)
Comment #12
plachOk, although not immediately
Comment #13
yched CreditAttribution: yched commentedTagging
Comment #14
yched CreditAttribution: yched commentedFrom @Berdir:
Comment #15
plachWorking on this.
Comment #16
plachThis should pass tests.
Comment #17
yched CreditAttribution: yched commentedWooohoooo ! Thanks a lot @plach !
Looks good to me, but I guess we need to find someone to RTBC that for us :-)
Comment #18
swentel CreditAttribution: swentel commentedLooks good indeed.
Comment #19
Berdir+1 to RTBC.
Unrelated, but is there already an issue for $field->isTranslatable()? If not then we should totally create one :)
Code uses offsetGet($delta) instead of [$delta] which is a bit longer which is a bit longer but possibly easier to follow. get($field_name) vs. {$field_name} is a bit inconsistent and I think I prefer get() with dynamic field names as {} stuff looks ugly. Downside is that get($field_name)[$delta] doesn't work but we don't use that anyway.
Maybe make that a bit more consistent before committing?
Comment #20
yched CreditAttribution: yched commented#19: true, here we are.
Comment #21
yched CreditAttribution: yched commentedTrue - dunno if there are plans already, but I opened #2018685: Remove field_is_translatable().
Comment #21.0
YesCT CreditAttribution: YesCT commentedtemplate and follow-up -yesct
Comment #21.1
YesCT CreditAttribution: YesCT commentedfound one I think might be blocked by this
Comment #21.2
YesCT CreditAttribution: YesCT commentedadded some detail about resolution. needs motivation spelled out. -yesct
Comment #21.3
YesCT CreditAttribution: YesCT commentedI think it relates to the issue where we introduced translation sync.
Comment #22
yched CreditAttribution: yched commentedFurther filled the summary.
Comment #23
YesCT CreditAttribution: YesCT commentedI looked at the code. Coding style wise looks great.
It seems to be removing BC stuff, which is great.
I'll update the issue summary, but it could use some help from someone more familiar with the entity field api stuff to make sure the motivation section is accurate.
Also, it looks like no api changes. So I'll put that in the summary too. (but correct it if I'm wrong)
[edit: yched updated those! :) ]
Small question:
Does this mean there is a entity field class for image fields?
What issue added that?
this is done a couple times, is it a nice improvement to tests to help make the output of the tests easier to understand? Looks like a good idea.
Comment #24
YesCT CreditAttribution: YesCT commentedQuestion just for my curiosity (not finding anything wrong).
phpstorm told me this is not used.
or is that the class used for this getField()?
Comment #25
alexpottCommitted b3f2fb1 and pushed to 8.x. Thanks!
Comment #26
yched CreditAttribution: yched commentedSorry for my "minor followup" spree - @YesCT was right in #24, needless "use" statement.
Comment #27
alexpottCommitted 8810170 and pushed to 8.x. Thanks!
Comment #28.0
(not verified) CreditAttribution: commentedFilled motivation & API changes