Spin off from #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title

Copied from over there:

Once #1969728: Implement Field API "field types" as TypedData Plugins is done, widgets and formatters are going to be the only thing that work with the old style $items arrays for field values.

Aside from API consistency, avoiding that conversion / format change would probably be a performance win - especially since :
- all those methods are called through a single set of helpers : field_invoke_method() / field_invoke_method_multiple() (they currently work only on Field API fields, that would be changed by making the EntityDisplay responsible for rendering)
- some of those methods (Widget::extractFormValues() & Formatter::prepareView()) do some writing to the $items, and thus the invoke helpers currently do the job of assigning the array values back into the FieldItem objects - for *all* methods, since they don't carry any knowledge of which methods write to the values or not.

This dance between "new (FieldItem objects) native format for field values" and "old (array) format for field values handed to widgets / formatters" is very far from ideal performance wise, and it possibly only works right now because there's an EntityBC layer that we switch the $entity to before calling the field_invoke_method() / field_invoke_method_multiple() helpers that bridge to the widgets & formatters. But that BC layer is intended to go away at some point...

It shouldn't be a very complex task in itself, but doing it on all formatters and widgets affects a lot of code...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Needs work » Needs review
Issue tags: +Field API, +sprint, +Spark, +Entity Field API
FileSize
21.42 KB

Attached is an initial patch that gets the ball rolling.

I'm going afk tomorrow til past code freeze :-/, so unfortunately I won't be able to push this myself further. So I just can hope that someone will pick the ball up :-)
AFAICT, the task shouldn't be too hard after the initial plumbing done here, the issue is mostly the amount of widgets and formatters, but this can easily be crowdsourced and / or worked on in parallel.

A word (hem) of explanation - sorry, a bit long, but terseness takes time, and I'm short on it :-).

The affected methods are:
Formatters:
- prepareView(),
- view() (+ the viewElements() sub method)
Widgets:
- form() (+ the formMultipleElements(), formSingleElement() sub methods)
- extractFormValues(),
- flagErrors()

They are all called through a common field_invoke_method() helper (field_invoke_method_multiple() for prepareView(), which acts on multiple entities). Thus, they all share a common signature (again, prepareView() is special) - currently :
method($entity, $langcode, array $items, $a, $b)
where:
- $items is an "old style" plain array of field values, keyed by delta.
- $a, $b are extra parameters specific to the method (possibly none). Typically, $form, $form_state for widget methods.

The goal is to change those signatures to:
method(Drupal\Core\Entity\Field\Type\Field $field, $a, $b)
where $field is the "EntityNG" Field values object. $entity and $langcode are not needed anymore, they can be read from the $field object.
And of course, change the methods content accordingly :-).

We'd need to make those new signatures become the "official" ones in FormatterInterface / WidgetInterface before July 1st.
But converting all core widgets & formatters to the new code might demand more time. So this patch implements a BC layer in the field_invoke_method*() helpers :-)

The idea is to support BC variants of each method, assuming methods in the interfaces have the "new" signature:

function field_invoke_method($method, $entity..., $a, $b) {
  foreach (field in the entity) {
    $formatter = ... determine the formatter object ... ("$target" in the actual function code)
    $methodBC = $method . 'BC';
    if (method_exists($formatter, $method)) {
      $formatter->$method($field_value_NG, $a, $b);
    }
    elseif (method_exists($formatter, $methodBC)) {
      $formatter->$methodBC($entity, $langcode, $items_BC, $a, $b);
    }
  }
}

(Because this is an initial patch and I didn't have time to update interfaces + append 'BC' to all methods of all existing widgets and formatters, the patch actually does the opposite : leave interfaces untouched, and check for 'NG' versions of the methods - but it needs to be the other way around in order to settle interfaces asap)

As an example, the patch also updates:
- TextDefaultFormatter::viewElements[NG]()
- TelephoneLinkFormatter::prepareView[NG]()
- WidgetBase::extractFormValues[NG]()

It also contains some very limited parts of #1969728: Implement Field API "field types" as TypedData Plugins needed while we still have "BC entity types" (Field::getValue(), Map::getValue(), exception commented out in TextProcessed::setValue())

It might even be green :-)
(this is very much "needs work", but setting to CNR for the fun...)

Status: Active » Needs work
Issue tags: -Field API, -sprint, -Spark, -Entity Field API

The last submitted patch, formatters_widgets_entityNG-2021817-1.patch, failed testing.

yched’s picture

effulgentsia’s picture

+++ b/core/modules/field/field.attach.inc
@@ -170,7 +170,79 @@ function field_invoke_method($method, $target_function, EntityInterface $entity,
+    // @todo Note: If we want make the new API official asap and port existing
+    // widgets / formatters progressively, this should be the other way around:
+    // - Update FormatterInterface & WidgetInterface method signatures
+    //   impacted methods:
+    //   Formatters:
+    //     prepareView(), view(), viewElements()
+    //   Widgets:
+    //     form(), extractFormValues(), flagErrors()
+    // - rename all existing implementations to [method]BC()
+    // - start by checking if method_exists($target, $method), else the
+    //   widget / formatter hasn't been converted yet, fallback to calling the
+    //   old $methodBC.
+    $methodNG = $method . 'NG';
+    if (method_exists($target, $methodNG)) {
+      $field = $instance->getField();
+      $field_name = $field->id();
+
+      // Determine the list of languages to iterate on.
+      $available_langcodes = field_available_languages($entity_type, $field);
+      $langcodes = _field_language_suggestion($available_langcodes, $options['langcode'], $field_name);
+
+      foreach ($langcodes as $langcode) {
+        // @todo getTranslation() only works on NG entities. Remove the
+        // condition and the second code branch when all core entity types are
+        // converted.
+        if ($translation = $entity->getTranslation($langcode)) {

We're so close to being able to drop the bc/ng craziness. #1983548: Convert contact message entities to the new Entity Field API is RTBC once we magically catch a bot that works, #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest is getting pretty close as well, and that's all that's left AFAIK.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets_entityNG-2021817-1.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

re #6: For EntityNG/EntityBC, yes. And yay !

Just to clarify though: this issue is about adding a "BC" layer for Widgets and Formatters, in the sense: "move their methods to a new signature, but keep supporting the "old" methods, so that we don't have to do all of them in one patch".
That "BC" layer is not related to EntityNG/EntityBC.

Now, in the code that supports the "new" methods, there is some code to deal with "EntityNG or EntityBC ?" (like, the couple last lines in the snippet pasted in #4). That specific code can go away when all entities are NG, yes.

Confusing, heh ?

The last submitted patch, formatters_widgets_entityNG-2021817-1.patch, failed testing.

effulgentsia’s picture

Let's see how badly we break without any BC layers. Just a straight conversion of array $items to FieldInterface $items.

Status: Needs review » Needs work
Issue tags: -Field API, -sprint, -Spark, -Entity Field API

The last submitted patch, formatters_widgets_entityNG-2021817-8.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, formatters_widgets_entityNG-2021817-8.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Field API, +sprint, +Spark, +Entity Field API

The last submitted patch, formatters_widgets_entityNG-2021817-8.patch, failed testing.

effulgentsia’s picture

Sigh. Not sure why bot is failing to login to the test site. I can run tests locally fine. Moving retests and debugging to #2014163: Bot checks for #2021817.

Wim Leers’s picture

I'll have very limited bandwidth for this in the next few days at least. I can promise reviews, I can't promise rerolls. I hope to play a more active role here ASAP.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
118.29 KB

This should be very close to green now and even if still failing a couple tests, is ready for review. Help with fixing the remaining test failures and/or reviews would be appreciated.

effulgentsia’s picture

Priority: Normal » Major

Escalating to major, since formatters and widgets are pretty fundamental to Drupal, and this changes their API in an important way per the issue summary.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets_entityNG-2021817-16.patch, failed testing.

yched’s picture

Priority: Major » Critical

Thanks so much for pushing this, Alex.

I can't back this up with actual numbers right now, but I'd be surprised if this doesn't critically affect performance, so - temptatively setting to 'critical' ?

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: -Field API, -sprint, -Spark, -Entity Field API

Status: Needs review » Needs work
Issue tags: +Field API, +sprint, +Spark, +Entity Field API

The last submitted patch, formatters_widgets_entityNG-2021817-16.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.13 KB
120.74 KB

Fixed the new failures related to the new Entity Translation API, but not the old failures.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets_entityNG-2021817-22.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
121.61 KB

Status: Needs review » Needs work

The last submitted patch, formatters_widgets_entityNG-2021817-24.patch, failed testing.

effulgentsia’s picture

The remaining failures don't happen for me locally. Do they for anyone else?

Berdir’s picture

Drupal\node\Tests\PagePreviewTest is also green for me. Maybe a PHP version thing? I have 5.4...

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: -Field API, -sprint, -Spark, -Entity Field API

Status: Needs review » Needs work
Issue tags: +Field API, +sprint, +Spark, +Entity Field API

The last submitted patch, formatters_widgets_entityNG-2021817-24.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
5.49 KB
125.91 KB

Maybe a PHP version thing? I have 5.4

Yep. Found the reason for that and added a comment about it. This also fixes some of the new failures from the recent retest, but possibly not all.

Status: Needs review » Needs work

The last submitted patch, formatters_widgets_entityNG-2021817-30.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
871 bytes
126.59 KB

Status: Needs review » Needs work
Issue tags: -Field API, -sprint, -Spark, -Entity Field API

The last submitted patch, formatters_widgets_entityNG-2021817-32.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: +Field API, +sprint, +Spark, +Entity Field API
effulgentsia’s picture

Yay! It's finally green. Please review.

fago’s picture

This looks great! A few questions:

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Formatter/FormatterBase.php
@@ -93,7 +94,8 @@ public function view(EntityInterface $entity, $langcode, array $items) {
+        // @todo Anything that uses #items should be converted as well...
+        '#items' => $items->getValue(TRUE),

Do we have a follow-up for that?

+++ b/core/modules/image/lib/Drupal/image/Tests/ImageFieldDefaultImagesTest.php
@@ -121,7 +121,7 @@ function testDefaultImages() {
-      $article_built[$field_name]['#items'][0]['fid'],
+      $article_built[$field_name]['#items'][0]['target_id'],

I'm wondering how that is related?

+++ b/core/modules/text/lib/Drupal/text/Plugin/field/formatter/TextDefaultFormatter.php
@@ -32,13 +33,16 @@
+      // @todo Convert text_sanitize() to work on an NG $item (this requires
+      //   http://drupal.org/node/1969728.)
+      $itemBC = $item->getValue(TRUE);
+      $output = text_sanitize($this->getFieldSetting('text_processing'), $langcode, $itemBC, 'value');

Please see #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property.

fago’s picture

Status: Needs review » Needs work
Issue tags: +Field API, +sprint, +Spark, +Entity Field API

The last submitted patch, formatters_widgets_entityNG-2021817-32.patch, failed testing.

yched’s picture

Thanks a ton @eff !
Reviewing this. Is this in a sandbox somewhere ?

yched’s picture

Status: Needs work » Needs review
FileSize
126.72 KB

Straight reroll, + pushed the code to the formatters_widgets_entityNG-2021817 branch in the sandbox

effulgentsia’s picture

Re #36.1: I opened #2042773: Change #items within a theme_field() render array from an *array* to the same $items *object* used throughout the rest of the formatter pipeline, but I suggest removing the @todo from the patch, since doing it is not strictly required. Or, @yched, if you want to keep the todo, please link it to that issue.

Re #36.2: So that the setValue() in ImageFormatterBase::prepareView() can set the 'target_id', which is what EntityReferenceItem expects.

Re #36.3: Let's update that @todo to point to #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property.

@yched: can you roll these quick @todo changes if you agree with them?

yched’s picture

Re-reroll

yched’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.79 KB
126.52 KB

- Adjusted the @todos as per #41
- minor simplifications in LinkFormatter::prepareView()

Other than that, reviewed the patch and have no other remarks - awesome !

Since the patch has now little in common with my initial proof of concept, and all I did since then was rerolls + the minor changes here, I feel I can RTBC this myself (if green, of course)

yched’s picture

Side note : I refrained myself from larger refactors of LinkFormatter, that are not really in scope here, and would have stopped me from RTBCing myself - opened #2047753: LinkFormatter::prepareView() is useless

Status: Reviewed & tested by the community » Needs work

The last submitted patch, formatters_widgets_entityNG-2021817-43.patch, failed testing.

yched’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1010 bytes
126.66 KB

Right, so of course @effulgentsia used this odd pattern in LinkFormatter for a reason...
We can't do $item->options['foo'] = 'bar', because $item->options is a magic property accessed through __get() / __set(), and PHP rejects the syntax ("Indirect modification of overloaded property Drupal\link\Type\LinkItem::$options has no effect")

Reverted those changes, so the interdiff with #42 is now purely adjusting @todo comments. Thus, back to RTBC.

This code can be simplified by getting rid of LinkFormatter::prepareView() as mentioned in #2047753: LinkFormatter::prepareView() is useless. Separate task.

Status: Reviewed & tested by the community » Needs work
Issue tags: -Field API, -sprint, -Spark, -Entity Field API

The last submitted patch, formatters_widgets_entityNG-2021817-46.patch, failed testing.

yched’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, formatters_widgets_entityNG-2021817-46.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
Issue tags: +Field API, +sprint, +Spark, +Entity Field API
fago’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Entity/Field/FieldItemBase.php
@@ -130,6 +130,7 @@ public function __isset($name) {
     $this->set($name, NULL);
+    unset($this->values[$name]);

Does it fail without that? If so, there must be bug in set(), not?

Except from that this looks great to me, so setting RTBC. Figuring out a possible set() issue should not hold this up and can be its own issue.

effulgentsia’s picture

Re #51, yeah something failed without that, but I forget what. #2014163-43: Bot checks for #2021817 will tell us.

Dries’s picture

Title: Make widgets / formatters work on EntityNG Field value objects. » [ChangeNotice] Make widgets / formatters work on EntityNG Field value objects.
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Great DX improvement. Committed to 8.x. Thanks.

Would be interested in learning about the performance gain that was hinted at.

xjm’s picture

Issue tags: +Approved API change
yched’s picture

Title: [ChangeNotice] Make widgets / formatters work on EntityNG Field value objects. » Make widgets / formatters work on EntityNG Field value objects.
Status: Active » Fixed
Issue tags: -Needs change record

Yay !

Updated the existing change notices
https://drupal.org/node/1805846 (formatters)
https://drupal.org/node/1796000 (widgets)

xjm’s picture

Issue tags: +Needs followup

Just saw #51 / #52 -- let's get a followup issue for that.

fago’s picture

Status: Fixed » Closed (fixed)

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

yched’s picture

Followup API change proposal - I should have spotted that earlier :-/
#2075095: Widget / Formatter methods signatures needlessly complex

yched’s picture

Issue summary: View changes

issue link