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);
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

alexpott’s picture

Issue tags: +Approved API change

Agree with the issue summary - doing this is a win for common sense and consistency

yched’s picture

Assigned: Unassigned » yched

I started this

yched’s picture

Assigned: yched » Unassigned
Status: Active » Needs review
FileSize
83.34 KB

Patch, let's see what the bot says.

#2061331: Add getEntity() helper in FieldInterface / FieldItemInterface would really make things nicer btw :-)

yched’s picture

Fix method calls in Field UI (default value widget)

Status: Needs review » Needs work

The last submitted patch, formatter_widget_simplify-2075095-5.patch, failed testing.

yched’s picture

Hah - 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...

yched’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
95.47 KB

This
- 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.

Status: Needs review » Needs work

The last submitted patch, formatter_widget_simplify-2075095-8.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
4.32 KB
96.91 KB

Posted 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.

Status: Needs review » Needs work

The last submitted patch, formatter_widget_simplify-2075095-10.patch, failed testing.

yched’s picture

Status: Needs review » Needs work
Issue tags: -Field API, -Entity Field API, -Approved API change

The last submitted patch, formatter_widget_simplify-2075095-12.patch, failed testing.

yched’s picture

So, 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.

yched’s picture

Status: Needs work » Needs review

Note: patch contains bits of #2061331: Add getEntity() helper in FieldInterface / FieldItemInterface, we probably need that other one to get in first.

plach’s picture

Totally +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.

yched’s picture

yched’s picture

testbot robbed our tags in #13 - including "Approved API change" :-p

Status: Needs review » Needs work

The last submitted patch, formatter_widget_simplify-2075095-14.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
1.6 KB
103.31 KB

theme_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.

swentel’s picture

+++ b/core/includes/menu.inc
@@ -1275,31 +1274,13 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
+  // Load the menu item corresponding to the current page.
+  if ($item = menu_get_item($active_path)) {
     if (isset($max_depth)) {

Is 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 ? :)

dawehner’s picture

This feels like a missing rebase.

yched’s picture

Yup, didn't merge between pulling 8.x and diffing with it I guess.
Reroll / fully merged.

Status: Needs review » Needs work

The last submitted patch, formatter_widget_simplify-2075095-23.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
95.1 KB

Reroll after #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property.
Cannot reproduce the fails in #23, assuming testbot fluke.

yched’s picture

yched’s picture

FWIW, I think this should be ready now.

Status: Needs review » Needs work
Issue tags: -API change, -Field API, -Entity Field API, -Approved API change

The last submitted patch, formatter_widget_simplify-2075095-26.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Field API, +Entity Field API, +Approved API change
swentel’s picture

yched’s picture

Indeed, both are modifying TestFieldEmptyFormatter.
Merged.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Went 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 :)

andypost’s picture

big +1 on it!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
86.91 KB

Status: Needs review » Needs work
Issue tags: -API change, -Field API, -Entity Field API, -Approved API change

The last submitted patch, formatter_widget_simplify-2075095-35.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Field API, +Entity Field API, +Approved API change

#35: formatter_widget_simplify-2075095-35.patch queued for re-testing.

I think the testbot is having a bad day.

yched’s picture

Thanks @amateescu :-)

Updated patch to make use of the recently added getEntity() instead of getParent()

amateescu’s picture

Could you leave the ER 'autocomplete_path' hunk to #2084081: ER autocomplete doesn't work which is already rtbc? :P

Edit: typo.

yched’s picture

Oh, 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']).

Status: Needs review » Needs work

The last submitted patch, formatter_widget_simplify-2075095-40.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
87.38 KB

Reroll.

Can't seem to reproduce the fails locally.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC then

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

yched’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll +Avoid commit conflicts
FileSize
17.57 KB
94.18 KB

Reroll
+ 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.

webchick’s picture

Title: Widget / Formatter methods signatures needlessly complex » Change notice: Widget / Formatter methods signatures needlessly complex
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed and pushed to 8.x. Thanks!

This'll need a change notice.

yched’s picture

Issue tags: -Avoid commit conflicts

Yay !

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.

webchick’s picture

+1 sounds good.

yched’s picture

Title: Change notice: Widget / Formatter methods signatures needlessly complex » Widget / Formatter methods signatures needlessly complex
Status: Active » Fixed
Issue tags: -Needs change record

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

Anonymous’s picture

Issue summary: View changes

rephrase