Now that widgets are in good shape, time to start on formatters !
Core issue used for testbot runs (until we have a patch ready to push for review in the wild):
#1785690: Testbot helper issue for 'formatters as plugins'
I extracted the code I had in field-plugins-yched, and adapted it to match the current design of the widgets code.
It lives in the field-plugins-formatters-1785748 branch.
The FieldInstance::getFormatter() method is a little more involved than getWidget(), since an $instance holds several formetters, and because of nice things like field_view_field($arbitrary_display_settings), and $display['type'] = 'hidden'.
Also, the prepareView() method being 'multiple' (acts on a array of $entities) adds some fun logic.
Only text formatters are migrated for now. A legacy layer, quite similar to the one we put for widgets, catches the other ones.
To match the current state of widgets, we should also migrate number formatters and test formatters.
Subtasks
#1785752: Move test formatters to the new API
#1785750: Move number formatters to the new API
#1787236: Convert email module formatters to Plugin system
#1787238: Convert options module formatters to Plugin system
#1787242: Convert file module formatters to Plugin system
#1787246: Convert image module formatters to Plugin system
#1787248: Convert taxonomy module formatters to Plugin system
Comment | File | Size | Author |
---|---|---|---|
#77 | Drupal-followup-1785748-70.patch | 2.8 KB | aritra.ghosh |
#67 | 1785748-67.patch | 125.94 KB | swentel |
#53 | 1785748-53.patch | 126.34 KB | swentel |
#53 | interdiff.txt | 694 bytes | swentel |
#49 | field-plugins-formatters-1785748-49.patch | 126.32 KB | Stalski |
Comments
Comment #1
yched CreditAttribution: yched commentedAttached is a patch against field-plugins-widgets-1785256.
Note: i haven't been able to get testbots run on this yet, the bots look like they are on strike tonight.
Comment #1.0
yched CreditAttribution: yched commentedadded real meat
Comment #1.1
yched CreditAttribution: yched commentedadded testbot issue
Comment #1.2
yched CreditAttribution: yched commentedadded formatting
Comment #2
yched CreditAttribution: yched commentedPushed :
- e8783c6: stop pre-filling display settings for all view modes in _field_info_prepare_instance()
Comment #3
pcambraTwo minor things in there
typo: used by
Possible inconsistence in the comments, in other formatters the whole class name is informed, e.g. Implements Drupal\field\Plugin\Type\Formatter\FormatterInterface::viewElements()
Comment #4
yched CreditAttribution: yched commentedPushed :
- #1785752: Move test formatters to the new API by pcambra
- fixes for the remarks in #3 - thanks !
Updated patch against field-plugins-widgets-1785256
Comment #5
yched CreditAttribution: yched commentedPushed:
- merged latest field-plugins-widgets-1785256 - includes:
latest 8.x
do not include upstream fixes for #1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result", but work around by overriding PluginManagerBase::getDefinition() in WidgetPluginManager.
- c0f5d7a: Added same workaround in FormatterPluginManager
Comment #6
tim.plunkettThis looks great, here are some code style nitpicks that aren't urgent, but could be kept in mind going forward:
Might as well add the type hints here and for newly added functions/methods
ID, not id. (Here and other places)
These should mention their default values.
The hyphens should be indented one more space, and there shouldn't be quotes around the key names
As I understood it, this either had to be
use
'd at the top of the file, or have an inline leading \ per #1614186: Coding standards for using "native" PHP classes like stdClass in namespaced codeShould have a blank line after the method (Here and other places)
Comment #7
yched CreditAttribution: yched commentedThanks Tim.
Most of those remarks actually apply to #1785256: Widgets as Plugins as well, so :
- fixed those upstream where applicable and rerolled #1785256: Widgets as Plugins
- pushed fixes for the formatter-specific bits :
71d8db3
76b2df4
Comment #8
yched CreditAttribution: yched commentedOops, patch was rolled against 8.x rather than field-plugins-widgets-1785256.
Rerolled.
Comment #9
yched CreditAttribution: yched commented- Fixed a couple test fails
- Lots of phpdoc fixes, pointed by@Lars Toomre
Comment #9.0
yched CreditAttribution: yched commentedAdded subtasks
Comment #10
yched CreditAttribution: yched commentedFixes last test notices, we should be green now.
Comment #11
Lars Toomre CreditAttribution: Lars Toomre commentedIf this patch is re-rolled, here are some additional documentation notes. Otherwise, the enhanced docblocks are much better than before!!
This comment needs to reformated to wrap at or before column 80.
Doesn't this also require a docblock?
Not sure why $entity_type is referenced here... Is this correct?
All of these need docblocks explaining the variable type and type hint.
Needs a docblock.
Other constructor methods in this patch do not have docblocks. I am unsure on what the documentation policy is, but we should be consistent throughout this patch.
I believe all of these variables need @param directives with type hinting and explanations. Also needs @return for $elements variable.
This needs @param and @return directives.
Missing docblocks....
Missing docblocks...
Comment #12
Lars Toomre CreditAttribution: Lars Toomre commented@yched... I am not sure what happened in #10, but patch is zero bytes.
Comment #13
yched CreditAttribution: yched commentedSome of #11 applies to this issue, some apply to #1785256: Widgets as Plugins.
I updated the patches to take those into account.
Updated formatters patch attached.
Comment #14
Lars Toomre CreditAttribution: Lars Toomre commented@yched: I am not sure if I am in the right issue, but here is my review of your patch in #13.
I know that it is not part of our coding standards, but it would be super helpful as a programmer to know what was expected for each of these array elements.
Does it make sense to do something like - entity_type: (string) The entity type ...
Based on the current documentation, I am unsure of what is expected for 'instance' key.
Both of these declarations should start with '(optional)'. Actually same goes for the $options variable as well.
Can we add a type hint here for consistency?
Can you expand upon this comment? Very unclear what you mean...
I am pretty sure that keys of an array should not be surrounded by single quotes.
This should reference what the default result is.
Small nit... ends in double periods, should be a single period.
This should be rewrapped for 80 character maximum.
For consistency, this should be 'FormatterInterface|null'
Not sure, but should not all use declarations be in alphabetical sort order?
Silly question from reading code comments... Can this summary string include HTML tags or is it expected to be plain text?
Unclear what is meant by 'If no language is provided'. First, there should be a comma after provided. However, not clear what happens if for instance $langcode is set to NULL in call.
Same comment as above about ordering of multiple use statements. Alphabetical?
Should not this @todo remain?
Comment #15
yched CreditAttribution: yched commentedHome for a couple days between two flights :-)
- Rerolled on current 8.x (includes widgets as plugins and FieldInfo changes)
- Merged #1785750: Move number formatters to the new API by pcambra, with a couple refactoring from myself.
- Added a couple doc adjustments after #14
Comment #16
swentel CreditAttribution: swentel commented@yched - when you're off again, let us know if there's anything we should focus on while you're gone so we can bring this forward
Note - have you got my mail re: how to arrange the train/flight stuff easily - man that sound stupids to type that here :)
Comment #17
yched CreditAttribution: yched commentedI think the only blocker now is some performance checks.
I guess relevant testing cases would be :
one node
10 simple text fields,
5 values in each field.
10 teasers of nodes of the same node type,
10 simple text fields in each node,
5 values in each field.
10 teasers of nodes of 10 different node types,
9 simple text fields and 1 taxonomy_term field in each node,
5 values in each field.
Last case is the worst case, because :
- different node types mean different $instances, and thus formatter objects are not reused across nodes, we instanciate more of them.
- taxonomy_term formatters have a 'prepare_view' step (taxonomy_field_formatter_prepare_view()) that does taxonomy_term_load_multiple(). The new architecture means prepare_view steps run in smaller groups (one per instance instead of one per formatter module), so that's more load_multiple() with less ids in each, which should have a perf impact that would be good to assess.
I'll see whether I can move forward on this before tuesday.
Comment #18
swentel CreditAttribution: swentel commentedAlright, I'll check the drush scripts I've already written, it already allows to create a content type with a random number of textfields, adding a taxonomy term field option should be easy, I'll try to update in an hour.
Comment #19
swentel CreditAttribution: swentel commentedDone, added option to generate term fields/instances as well. It uses the tags vocabulary that comes default with core install. makes it easier to setup scenarios faster.
command: drush fact 9 1
http://drupal.org/sandbox/swentel/1781486 (note, you're maintainer too, so feel free to make adjustments)
I'll run some tests tomorrow, rest of the day is dedicated to do nothing :)
Comment #20
yched CreditAttribution: yched commentedUpdated patch - mostly :
- de7b973: forward-port the same workaround that was added in #1751234: Convert option widgets to Plugin system in the LegacyDiscovery to allow info_alter hooks on new-style widgets
- 141001d thus, we can remove the hack in mail field tests (mail fields use formatter_info_alter to use text formatter as their default formatter)
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedWhat's left before this issue can be moved to the core queue? Would be great to get a bot check plus increase visibility.
Comment #22
yched CreditAttribution: yched commentedre @effulgentsia :
This should be mostly ready, aside from possible doc / code style nitpicks.
Patch is green (I've been using #1785690: Testbot helper issue for 'formatters as plugins' to run bots while we are in the sandbox)
Last thing we'd need is performance checks, cause moving from "one hook does the job of all formatters in a module" to "one formatter object per field instance" has inherent performance costs (more function calls, more objects instantiation, less grouping of multiple loads in perpare_view).
I've outilined testing cases in #17 above, but I'll probably go afk before I get a chance to run them myself :-/
Comment #23
effulgentsia CreditAttribution: effulgentsia commentedSounds to me like this should be in the core queue then, so doing so. Please reset if I'm wrong though.
Comment #24
Lars Toomre CreditAttribution: Lars Toomre commented@yched when would you like a detailed review for possible doc / code style issues?
Comment #25
yched CreditAttribution: yched commented@Lars Toomre: thanks for the offer :-). Preferably after the perf test results validate the architecture.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedPer #22, this patch is nearly done, so I think a docs and code style review would be appropriate now, unless yched says otherwise. Also, this issue isn't blocking any other major or critical issues that I'm aware of, so incorporating that feedback should be done as part of this issue, not a follow up.
Comment #27
swentel CreditAttribution: swentel commentedDid some basic performance tests, not being able todo all the use cases though, and no xhprof so far, I'll try todo one before wednesday, but then I'm afk after that until next monday as well. The cool thing is that we already profit from the _field_info_collate_patch() that got in :)
I have a public dump available if anyone in the meantime wants to have a go: http://dl.dropbox.com/u/15116672/dump-d8.zip
It contains 10 content types, each with 9 text fields and 1 term field. It has a lot of terms and 5000 nodes.
Single node view
before: Page execution time was 65.91 ms. Memory used at: devel_boot()=1.2 MB, devel_shutdown()=4.89 MB, PHP peak=5 MB.
after: Page execution time was 68.29 ms. Memory used at: devel_boot()=1.24 MB, devel_shutdown()=5.07 MB, PHP peak=5.25 MB.
Front page (I think it lists 7 different node types now)
before: page execution time was 141.57 ms. Memory used at: devel_boot()=1.24 MB, devel_shutdown()=7.55 MB, PHP peak=9.25 MB.
after: page execution time was 143.88 ms. Memory used at: devel_boot()=1.24 MB, devel_shutdown()=7.85 MB, PHP peak=9.5 MB.
So far, not really alarming if you tell me.
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedThanks. 68.29 vs. 65.91 = 3.6%, which is enough to warrant digging into.
Comment #29
yched CreditAttribution: yched commentedThanks @swentel ! I hope I can give a deeper look with xhprof tonight,
From what we've seen in the "widgets as plugins" patch, I kind of hope those 3% are mostly caused the class autoloading overhead - "hooks -> plugins" inherently means more classes to load, and we know our autoloader is slow.
Comment #30
Lars Toomre CreditAttribution: Lars Toomre commented@yched - Happy to help. Let me know when! I really like your work here.
Comment #31
pounardNote about the patch: type hinting with
\Closure
is a really bad idea. Closures were at the origin an implementation detail not supposed to be public, even if it may be now, it's still more flexible to type hint usingcallbable
instead. Note that the callable type hint is not permited until 5.4 but at least you can document it as a callable and just not type hint it.Comment #32
Lars Toomre CreditAttribution: Lars Toomre commentedIf callable is not available until 5.4, it is inappropriate to use while Drupal requires 5.3.x. In my mind, type hinting is a MUST for all new code being added to D8.
If you think Closure is wrong to use as a type hint, open up an issue to change it once 5.4 becomes a minimal requirement for Drupal. Until then, this should be type hinted as accurately as possible.
Comment #33
yched CreditAttribution: yched commentedSo, those were the numbers I found using @swentel's db dump in #27:
- /node/%nid (9 text fields, 1 taxo field)
before: 97.081 ms / after: 99.295 ms (+2%)
XHProf results:
overhead in node_show() : +5.711 (patch only affects the callstack below)
with:
UniversalClassLoader::loadClass(): +3.809
ReflectionFactory: +1.200
Which pretty much adds up.
- /node (10 nodes, 10 fields each: 9 text + 1 taxo)
before: 247.598 ms / after: 259.321 ms (+4.7%)
XHProf results:
overhead in node_view_multiple(): +16.005 (patch only affects the callstack below)
with:
UniversalClassLoader::loadClass(): +3.584
ReflectionFactory: +8.927
Which leaves ~3.500 ms that I wasn't really able to track down to a specific place.
So:
- Silly me for forgetting to add a dedicated FormatterFactory instead of Reflectionfactory, like we did for widgets. Attached patch adds it.
See commitdiff.
- The rest of the overhead is mostly spent in the ClassLoader.
Unfortunately, my poor laptop seems to have overheated after that, and doesn't produce consistent before / after benchmarks for this new version of the patch. Will retry tomorrow.
Comment #34
pounardWe technically can't type hint here, because it makes impossible to use any other form of callable. This can't be commited with a \Closure type hint.
PHP has technical limitations, and this is one <5.4 specific, we must live with it: you still don't type hint for strings and integers right? Then the same behavior for callables apply, don't type hint.
If we pass a callable we must not type hint else we loose great flexibility for no reason. We can replace the type hint by:
if (!is_callable($my_param)) { throw new \InvalidArgumentException(); }
in place to reproduce the fatal behavior of the wrong call.Comment #35
yched CreditAttribution: yched commentedUpdated benchmarks with patch in #33:
- /node/%nid (9 text fields, 1 taxo field)
after: 84.139 ms / after: 85.101 ms (+1.1%)
- /node (10 nodes, 10 fields each: 9 text + 1 taxo)
before: 192.044 ms / after: 194.785 ms (+1.4%)
As explained in #33, and as was the case for "widgets as plugins", the overhead mostly comes from the autoloader.
So, I think we're ready !
Comment #36
Lars Toomre CreditAttribution: Lars Toomre commentedReady for a detailed code /doc review too?
Comment #37
yched CreditAttribution: yched commented@Lars Toomre : yup :-)
Comment #38
yched CreditAttribution: yched commentedre #34: makes sense I guess. I'll reroll later today removing the Closure type hint.
Comment #39
Lars Toomre CreditAttribution: Lars Toomre commentedHere is what I observed in #33. Please realize that is intended to be a nit picking code / documentation style review.
Just to be consistent, let's change 'entity type;' to 'entity type:'.
The variable used in the function call is $display. This needs to be adjusted.
Are use statements suppose to be orders in alphabetical order?
Glad that this is type hinted. I would have guessed that plugin_id was a string otherwise.
I think this needs to be 'Returns'.
I think this needs to be 'Allows'.
I think that these should be sorted into alphabetical order.
Ibid.
Ibid.
Ibid.
Comment #40
tim.plunkettActually, they should both be commas.
Please stop posting this on issues, it's not in our coding standard. They can be in any order.
Comment #41
yched CreditAttribution: yched commentedThanks for the review, Lars.
Consistent with what ? AFAIK, we're rather moving away from those array keys with spaces in them, so I'm for keeping this as is, esp. if it avoids a needless API break.
Actually, that was the case in the old field_get_display() function, that is not used anymore, but the new calling code (in FieldInstance::getFormatter()) is actually correct. Updated patch removes field_get_display().
- Fixed the other remarks (except those about the ordering of 'use' statements)
- Removed the '\Closure' type hint in field_invoke_method() and field_invoke_method_multiple(), and use 'callable' in the phpdoc, as per #34. Also improved consistency of the variable names around there.
Interdiff with #33 attached.
Comment #42
yched CreditAttribution: yched commentedOops, sorry, finally got what you meant about 'entity type:'
Fixed.
Comment #43
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @yched. The docs look much good.
Comment #44
yched CreditAttribution: yched commentedRe-title to match #1785256: Widgets as Plugins
Comment #45
pounard#42 patch still includes Closure type hint and documentation in some various points (in @return statement for example).
Another minor, but yet important detail: if you use callable as type hint (at least as document type for now) you should call it using call_user_func($var, $a1, $a2) instead of direct $var($a, $a2); Performance penalty will be almost insignificant and it will avoid PHP WSOD if you pass there an array($object, 'methodName') instead of a Closure object.
Comment #46
yched CreditAttribution: yched commentedRe #45: Ack.
However, I won't be able to easily reroll patches for the next couple weeks.
Anyone ? :-)
Comment #47
yched CreditAttribution: yched commentedAlso, #1705702: Provide a way to allow modules alter plugin definitions got in, which means HookDiscovery no longer runs the hook_field_formatter_info_alter().
- AlterDecorator needs to be added to the discovery used in FormatterPluginManager,
- and the workaround in LegacyDiscoveryDecorator can be removed (denoted by a @todo)
I'm not able to roll patches right now - anyone up for it ?
Comment #48
Stalski CreditAttribution: Stalski commentedI'll reroll it today and enforce the suggested changes.
Comment #49
Stalski CreditAttribution: Stalski commentedinto
Tried to create interdiff as well but then I needed to do the reroll again.
SideQuestion:
Are there already issues created to let widgets use the AlterDecorator too? maybe some other stuff too?
Comment #50
Stalski CreditAttribution: Stalski commentedComment #51
swentel CreditAttribution: swentel commentedGreat! @pounard - if you're ok with this one, can you push RTBC ? Or let us know where changes are needed ? Thanks!
Comment #52
yched CreditAttribution: yched commentedfield_invoke_method_multiple() needs call_user_func() as well :-)
Comment #53
swentel CreditAttribution: swentel commentedThis ?
Comment #54
Stalski CreditAttribution: Stalski commentedNice swentel, that's it!
@yched, thanks again!
Really hope it's ready to go now.
Comment #55
pounardI won't RTBC it because I prefer to let people that know better the field API do it. Just know that callable type hint sounds fine now and I'm OK with the latest patch regarding that specific point.
Comment #56
swentel CreditAttribution: swentel commented@pounard fair enough, thanks for the OK on the remarks you had :)
Maybe we can create a change record modelled after http://drupal.org/node/1796000 in the mean time - we can safely assume this will go in (and I can remove the change record afterwards in case this won't make it.).
edit: change notice at http://drupal.org/node/1805846
Comment #57
andypostThe patch is well-tested in sandbox and manually so some clean-ups could be done in follow-ups. Let's get them commited
Comment #58
yched CreditAttribution: yched commented@andypost: why "needs committer feedback"?
Comment #59
andypost@yched because this change requires @catch feedback according http://drupal.org/node/1207020
Comment #60
tim.plunkett"because the community was not able to come to a consensus and needs a top-down decision"
This is RTBC, that on its own means the committers will look at it.
Comment #61
yched CreditAttribution: yched commented#1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result" got in, so we can remove the workaround in FormatterPluginManager (denoted by a @todo refenrencing the issue URL).
Doesn't do any harm, though, so keeping RTBC.
Comment #62
yched CreditAttribution: yched commentedOh, almost forgot :
@ core committers : @pcambra should be credited too in the commit message.
Comment #63
attiks CreditAttribution: attiks commented@yched did you forget the patch?
Comment #64
andypost@yched The commited #1778942-14: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result" already have this hunk, so no need for re-roll
Comment #65
andypostYup, missed this
Comment #66
yched CreditAttribution: yched commentedYup. As I said, though, this does no harm right now and can still be removed in a followup. Thus, keeping RTBC.
Comment #67
swentel CreditAttribution: swentel commentedLet's do that now :)
Comment #68
swentel CreditAttribution: swentel commentedComment #69
kbasarab CreditAttribution: kbasarab commented#67: 1785748-67.patch queued for re-testing.
Comment #70
webchickOk sorry for the delay on this folks; this took some time to go through.
In general this looks very similar to the widgets patch, so once again, great work on the clean-up there.
I committed and pushed to 8.x, but had some follow-up questions/observations. Also tagging for a change notice.
Why remove entity here? That seems like valuable context. I noticed 'module' was also removed from another place (can't recall where, sorry).
I am really not a fan of these "mixed" type variables. Would it make sense here to make view mode (or whatever) an explicit function parameter?
woah. crazy syntax. didn't know you could do that. I can't quite parse what's happening there, so a comment would be great.
Comment #71
swentel CreditAttribution: swentel commentedChange notice is at http://drupal.org/node/1805846, so this is normal again.
Comment #72
swentel CreditAttribution: swentel commentedAlso, @yched, should we just bundle the plugins and widgets conversions into one issue per module ?
Comment #73
yched CreditAttribution: yched commented@webchick : yay, thacaanks. I will comment on your remarks when i can access a real keyboard in a couple days :-)
@swentel: some widgets (options, file...) are a bit complex, so my gut feeling would be to keep separate issues. But maybe for simple stuff like mail, url... we can group.
We should probably open a meta issue to track conversion tasks.
Comment #74
xjmBug with field access: #1814418: FormatterBase::view() calls field_access() with the wrong order of arguments
Comment #75
yched CreditAttribution: yched commentedChange notice has been taken care of.
Comment #76
yched CreditAttribution: yched commentedOops, followup for a bug this introduced in Field UI: #1848040: Fatal error on "Manage display" after creating new field
Comment #77
aritra.ghosh CreditAttribution: aritra.ghosh commentedProvided the patch to
- remove 'mixed' from @params in function header
- comments for the use of php closure
Comment #78
yched CreditAttribution: yched commentedSo, this is long overdue, but re: @webchick's #70
- $entity in hook_field_display_alter() / hook_field_display_ENTITY_TYPE_alter() :
Those hooks don't receive $entity anymore because they are not called on a per-entity basis anymore. This was just not doable efficiently. Formatter plugin objects are created and persisted through the request per field instance (i.e for all entities in a bundle). If every individual entity can come and specify they want a different formatter for the field, then we have no way to share formatter objects, and we would need to create one formatter for each field x each entity. Massive perf impact on listing pages.
This was discussed with @chx, who introduced the request for "per entity hook_field_display_alter()" in #830020: field_get_display does not get the entity, and it was deemed a reasonable regression. The use cases for "alter display settings for a field for a given entity" can use hook_entity_view_mode_alter(), that runs entity by entity.
- "mixed" parameter in _field_invoke_widget_target() / FieldInstance::getWidget():
That area gets refactored in #1852966: Rework entity display settings around EntityDisplay config entity, both functions disappear, and the "mixed" param too.
So I'd rather we leave it that way for now, or it will just cause yet another reroll in that other issue.
- Comment on the closure on _field_invoke_formatter_target()
As said above, this is refactored in #1852966: Rework entity display settings around EntityDisplay config entity, and a comment is added over there.
All in all,
- sorry for the delay :-/
- thanks @aritra.ghosh for patch #77, it is technically valid, but those points are adressed in #1852966: Rework entity display settings around EntityDisplay config entity, let's rather get that one in.
Thus, setting to 'fixed', we're done here.
Comment #79
yched CreditAttribution: yched commentedresetting title, too.
Comment #80
swentel CreditAttribution: swentel commentedRemoving tags as well
Comment #81.0
(not verified) CreditAttribution: commentedAdded new issues for converting the rest of formatters