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...
Comment | File | Size | Author |
---|---|---|---|
#46 | formatters_widgets_entityNG-2021817-46.patch | 126.66 KB | yched |
#46 | interdiff.txt | 1010 bytes | yched |
#43 | formatters_widgets_entityNG-2021817-43.patch | 126.52 KB | yched |
#43 | interdiff.txt | 2.79 KB | yched |
#42 | formatters_widgets_entityNG-2021817-42.patch | 126.74 KB | yched |
Comments
Comment #1
yched CreditAttribution: yched commentedAttached 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:
(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...)
Comment #3
yched CreditAttribution: yched commented#1: formatters_widgets_entityNG-2021817-1.patch queued for re-testing.
Comment #4
effulgentsia CreditAttribution: effulgentsia commentedWe'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.
Comment #6
yched CreditAttribution: yched commentedre #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 ?
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedLet's see how badly we break without any BC layers. Just a straight conversion of
array $items
toFieldInterface $items
.Comment #10
effulgentsia CreditAttribution: effulgentsia commented#8: formatters_widgets_entityNG-2021817-8.patch queued for re-testing.
Comment #12
effulgentsia CreditAttribution: effulgentsia commented#8: formatters_widgets_entityNG-2021817-8.patch queued for re-testing.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedSigh. 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.
Comment #15
Wim LeersI'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.
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedThis 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.
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedEscalating to major, since formatters and widgets are pretty fundamental to Drupal, and this changes their API in an important way per the issue summary.
Comment #19
yched CreditAttribution: yched commentedThanks 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' ?
Comment #20
effulgentsia CreditAttribution: effulgentsia commented#16: formatters_widgets_entityNG-2021817-16.patch queued for re-testing.
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedFixed the new failures related to the new Entity Translation API, but not the old failures.
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedComment #26
effulgentsia CreditAttribution: effulgentsia commentedThe remaining failures don't happen for me locally. Do they for anyone else?
Comment #27
BerdirDrupal\node\Tests\PagePreviewTest is also green for me. Maybe a PHP version thing? I have 5.4...
Comment #28
effulgentsia CreditAttribution: effulgentsia commented#24: formatters_widgets_entityNG-2021817-24.patch queued for re-testing.
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedYep. 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.
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedComment #34
effulgentsia CreditAttribution: effulgentsia commented#32: formatters_widgets_entityNG-2021817-32.patch queued for re-testing.
Comment #35
effulgentsia CreditAttribution: effulgentsia commentedYay! It's finally green. Please review.
Comment #36
fagoThis looks great! A few questions:
Do we have a follow-up for that?
I'm wondering how that is related?
Please see #2026339: Remove text_sanitize() since we have TextProcessed as an EntityNG computed property.
Comment #37
fago#32: formatters_widgets_entityNG-2021817-32.patch queued for re-testing.
Comment #39
yched CreditAttribution: yched commentedThanks a ton @eff !
Reviewing this. Is this in a sandbox somewhere ?
Comment #40
yched CreditAttribution: yched commentedStraight reroll, + pushed the code to the formatters_widgets_entityNG-2021817 branch in the sandbox
Comment #41
effulgentsia CreditAttribution: effulgentsia commentedRe #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?
Comment #42
yched CreditAttribution: yched commentedRe-reroll
Comment #43
yched CreditAttribution: yched commented- 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)
Comment #44
yched CreditAttribution: yched commentedSide 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
Comment #46
yched CreditAttribution: yched commentedRight, 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.
Comment #48
yched CreditAttribution: yched commented#46: formatters_widgets_entityNG-2021817-46.patch queued for re-testing.
Comment #50
yched CreditAttribution: yched commented#46: formatters_widgets_entityNG-2021817-46.patch queued for re-testing.
Comment #51
fagoDoes 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.
Comment #52
effulgentsia CreditAttribution: effulgentsia commentedRe #51, yeah something failed without that, but I forget what. #2014163-43: Bot checks for #2021817 will tell us.
Comment #53
Dries CreditAttribution: Dries commentedGreat DX improvement. Committed to 8.x. Thanks.
Would be interested in learning about the performance gain that was hinted at.
Comment #54
xjmComment #55
yched CreditAttribution: yched commentedYay !
Updated the existing change notices
https://drupal.org/node/1805846 (formatters)
https://drupal.org/node/1796000 (widgets)
Comment #56
xjmJust saw #51 / #52 -- let's get a followup issue for that.
Comment #57
fagoIndeed, opened #2050201: Unsetting field item properties via FieldItemBase::set() does not always work.
Comment #59
yched CreditAttribution: yched commentedFollowup API change proposal - I should have spotted that earlier :-/
#2075095: Widget / Formatter methods signatures needlessly complex
Comment #59.0
yched CreditAttribution: yched commentedissue link