Widget & formatter methods typically receive an $entity, a $langode, and field values as $items:
FormatterInterface::prepareView(array $entities, $langcode, array $items);
FormatterInterface::view(EntityInterface $entity, $langcode, FieldInterface $items);
WidgetInterface::extractFormValues(EntityInterface $entity, $langcode, FieldInterface $items, array $form, array &$form_state)
WidgetInterface::formElement(FieldInterface $items, $delta, array $element, $langcode, array &$form, array &$form_state);
Those $entity & $langode params are unnecessary clutter:
- They can now be accessed through $items (already possible in HEAD, #2061331: Add getEntity() helper in FieldInterface / FieldItemInterface makes it a tad easier)
This is what all the rest of HEAD that works with Field / FieldItems does, having them handed out here as separate params (as if they were different from the ones that can be found in the $items) is a WTF.
- Very little widget / formatter code actually needs them (do stuff differently depending on the $entity or $langcode), and the code that does need them can get them.
- They obfuscate the simplicity of the methods tasks : "do your stuff on those $items"
The above could be simplified to just:
FormatterInterface::prepareView(array $items);
FormatterInterface::view(FieldInterface $items);
WidgetInterface::extractFormValues(FieldInterface $items, array $form, array &$form_state)
WidgetInterface::formElement(FieldInterface $items, $delta, array $element, array &$form, array &$form_state);
Comment | File | Size | Author |
---|---|---|---|
#45 | formatter_widget_simplify-2075095-45.patch | 94.18 KB | yched |
#45 | interdiff.txt | 17.57 KB | yched |
#42 | formatter_widget_simplify-2075095-42.patch | 87.38 KB | yched |
#40 | formatter_widget_simplify-2075095-40.patch | 87.38 KB | yched |
#40 | interdiff.txt | 1.44 KB | yched |
Comments
Comment #1
BerdirSee also #2073217: Remove the $langcode parameter from the entity view/render system, where I noticed this too.
Comment #2
alexpottAgree with the issue summary - doing this is a win for common sense and consistency
Comment #3
yched CreditAttribution: yched commentedI started this
Comment #4
yched CreditAttribution: yched commentedPatch, let's see what the bot says.
#2061331: Add getEntity() helper in FieldInterface / FieldItemInterface would really make things nicer btw :-)
Comment #5
yched CreditAttribution: yched commentedFix method calls in Field UI (default value widget)
Comment #7
yched CreditAttribution: yched commentedHah - of course, this is why it hasn't been done as part of #2021817: Make widgets / formatters work on EntityNG Field value objects..
So the patch results in widgets being placed at $form['field_foo']['en'] instead of $form['field_foo']['und'] for non-translatable fields in node forms, which breaks a lot of drupalPost() calls in tests.
That's because field.multilingual considers the field language to be 'und' (field is not translatable), while the entity language (for a node created *in a form*) is 'en'. I opened #2076445: Make sure language codes for original field values always match entity language regardless of field translatability about this.
So the language currently passed by field_invoke_method() / field.multilingual to widget methods *is* actually different from the language that can be found in the FieldItems for node forms. By using the latter rather than the former, the patch changes the structure of node forms.
This makes me even more convinced that FieldItems should know their own language, rather than having a separate $langcode param passed along to code that work on them - and this is what the patch does.
But I'm very confused ATM as to how we go forward here. Multilingual entity logic is really in flux right now:
#1810370: Entity Translation API improvements is still fresh,
#2019055: Switch from field-level language fallback to entity-level language fallback is reshuffling things,
and field.multilingual.inc is considered mostly obsolete (#2067079: Remove the Field Language API) but still governs how configurable fields work right now.
We *could* manually change each test that posts to node forms to account for the new form structure ($form['field_foo'][$entity_language] rather than $form['field_foo']['und']), but I don't know if that would be the final intended behavior for field / entity translation...
Comment #8
yched CreditAttribution: yched commentedThis
- injects the langcode in each Field value object,
- adds a getLangcode() method on Field & FieldItem
- uses this langcode in widgets & formatters instead of the langcode of the entity
The first two items deserve their own issue, but let's see how this goes.
No easy interdiff because of merges, but the interesting new code is attached.
Comment #10
yched CreditAttribution: yched commentedPosted a separate patch for Field/FieldItem::getLangcode() method at #2078625: Field/FieldItem value objects should carry their language
Meanwhile, this is also included in the patch here.
This should fix a bunch of fails - some places were still using the langcode of the entity.
Comment #12
yched CreditAttribution: yched commentedReroll - #2078625: Field/FieldItem value objects should carry their language got in
+ Fix "default image" fails (see #2074217-22: Reconsider support for field-level & instance-level settings with the same name ?)
Comment #14
yched CreditAttribution: yched commentedSo, this is where things get funny.
The Entity API doesn't use the same language logic than field.multilingual.inc. So, by moving widgets to use the language determined by the Entity API for the $items objects, rather than the one determined by the old field.multilingual.inc, this patch changes the language keys found in the form structures.
- HEAD: widget for 'body' in a french node is at $form['body']['fr']
- patch: widget for 'body' in a french node is at $form['body']['und'],
since Entity API considers fields for the default language of the entity to be language neutral...
Patch thus needs to update the tests that post to the old form structures:
CommentLanguageTest, TranslationWebTest, NodeFieldMultilingualTestCase.
Note that this actually doesn't change anything, the langcode keys in the forms are basically useless now, and do not determine in which langode the submitted values are stored: WidgetBase::extractFormValues() assigns values into the $items object that gets passed to it, how the form is structured doesn't change a thing.
In fact, IMO:
- We should get rid of those langcode subkeys in the $form structure, we're not editing entities in multiple languages in the same form, core entity form controllers edit an entity in one single language. Within that language, some fields are translatable and some are not, that's it. It doesn't make a difference for the form structure.
- Even if some contrib module wanted to do "side editing in two languages in one form", it's then its responsibility to structure those forms accordingly.
At any rate, it's not the job of *widgets* to spit form $elements nested below a langcode subkey, as they do currently (see the end of WidgetBase::form()). Widgets generate "what it takes to edit this field", that's it. Multilingual structure, *if needed*, should be added on top of the result of $widget::form() by the calling code.
Removing those langocde subkeys would mean updating a lot more tests, and is outside the scope of this patch, though.
Comment #15
yched CreditAttribution: yched commentedNote: patch contains bits of #2061331: Add getEntity() helper in FieldInterface / FieldItemInterface, we probably need that other one to get in first.
Comment #16
plachTotally +1 on removing the langcode level in the form widget structures, it has always been a major source of troubles. I agree that for advanced use cases the implementing module should build its form(s) by assembling form widgets or possibily entire entity forms with their respective languages, as suggested in #1728816: Ensure multiple entity forms can be embedded into one form.
Comment #17
yched CreditAttribution: yched commented@plach: way cool :-) Opened #2083811: Remove langcode nesting for fields in entity $form structures
Comment #18
yched CreditAttribution: yched commentedtestbot robbed our tags in #13 - including "Approved API change" :-p
Comment #20
yched CreditAttribution: yched commentedtheme_field() fails to output anything if $items is empty, so TestFieldEmptyFormatter does need to add stuff in $items. That's sick, but not for this issue to solve. Reverted my refactoring of TestFieldEmptyFormatter, and opened #2083835: theme('field') displays no value on empty fields even if the formatter returned an output for this.
Comment #21
swentel CreditAttribution: swentel commentedIs this, and other changes in menu.inc, related to this patch ? I mean, sure we should remove the old router at some point, but that's not up to us ? :)
Comment #22
dawehnerThis feels like a missing rebase.
Comment #23
yched CreditAttribution: yched commentedYup, didn't merge between pulling 8.x and diffing with it I guess.
Reroll / fully merged.
Comment #25
yched CreditAttribution: yched commentedReroll after #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property.
Cannot reproduce the fails in #23, assuming testbot fluke.
Comment #26
yched CreditAttribution: yched commentedReroll after #2061331: Add getEntity() helper in FieldInterface / FieldItemInterface
Comment #27
yched CreditAttribution: yched commentedFWIW, I think this should be ready now.
Comment #29
yched CreditAttribution: yched commented#26: formatter_widget_simplify-2075095-26.patch queued for re-testing.
Comment #30
swentel CreditAttribution: swentel commented#2083835: theme('field') displays no value on empty fields even if the formatter returned an output got in, does it affect this patch ?
- edit - wrong issue pasted
Comment #31
yched CreditAttribution: yched commentedIndeed, both are modifying TestFieldEmptyFormatter.
Merged.
Comment #32
swentel CreditAttribution: swentel commentedWent through this a couple of times, looks good.
We can update https://drupal.org/node/1805846 and https://drupal.org/node/1796000 after commit - and also the blog posts on my site :)
Comment #33
andypostbig +1 on it!
Comment #34
alexpottPatch no longer applies.
Comment #35
amateescu CreditAttribution: amateescu commentedConflicted a lot with #2083811: Remove langcode nesting for fields in entity $form structures, hope I got`em all.
Comment #37
amateescu CreditAttribution: amateescu commented#35: formatter_widget_simplify-2075095-35.patch queued for re-testing.
I think the testbot is having a bad day.
Comment #38
yched CreditAttribution: yched commentedThanks @amateescu :-)
Updated patch to make use of the recently added getEntity() instead of getParent()
Comment #39
amateescu CreditAttribution: amateescu commentedCould you leave the ER 'autocomplete_path' hunk to #2084081: ER autocomplete doesn't work which is already rtbc? :P
Edit: typo.
Comment #40
yched CreditAttribution: yched commentedOh, sure, wasn't aware of that other issue, sorry.
Left a comment over there, reverted my changes here (other than using getEntity() rather than $element['#entity']).
Comment #42
yched CreditAttribution: yched commentedReroll.
Can't seem to reproduce the fails locally.
Comment #43
yched CreditAttribution: yched commentedBack to RTBC then
Comment #44
webchickPatch no longer applies.
Comment #45
yched CreditAttribution: yched commentedReroll
+ removes a bunch of now needless "use EntityInterface" statements in the formatter classes, the method signatures do not contain an entity anymore. Patch already cleaned up widgets similarly, but I forgot to do the same when updating formatters.
Comment #46
webchickCommitted and pushed to 8.x. Thanks!
This'll need a change notice.
Comment #47
yched CreditAttribution: yched commentedYay !
There are already change notices for the formatter / widget D8 APIs, so I think we want to update those instead of creating a new one ?
https://drupal.org/node/1805846
https://drupal.org/node/1796000
I'll do that later today.
Comment #48
webchick+1 sounds good.
Comment #49
yched CreditAttribution: yched commentedUpdated the change notices:
Formatters: https://drupal.org/node/1805846/revisions/view/2782581/2842743
Widgets: https://drupal.org/node/1796000/revisions/view/2788631/2842759
Comment #50.0
(not verified) CreditAttribution: commentedrephrase