Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#24 | field-formatter-1985344-24.patch | 39.17 KB | swentel |
#24 | interdiff.txt | 633 bytes | swentel |
#23 | field-formatter-1985344-23.patch | 39.17 KB | swentel |
#21 | field-formatter-1985344-21.patch | 39.14 KB | swentel |
#21 | interdiff.txt | 655 bytes | swentel |
Comments
Comment #1
yched CreditAttribution: yched commentedFWIW, the documentation for the plugin definition entries was formerly in the phpdoc for hook_field_formatter_info(), which got removed in #1785748: Field formatters as plugins. The latest state of the phpdoc can be seen in the commitdiff.
Comment #2
tim.plunkettFirst pass.
Comment #4
tim.plunkettHeh, forgot all of the test formatters.
Comment #5
yched CreditAttribution: yched commentedLooks good to me, aside from:
So what do we do with this ? Move those to an alter implementation in edit.module ?
Comment #6
swentel CreditAttribution: swentel commentedThat would make sense in a way. Otoh, the edit module will probably need to have some way to determine whether it will be using 'direct' or 'form' (are there more?), otherwise we'll end up with a lot of hardcoded stuff in that alter.
Comment #7
Wim LeersIt is unacceptable (at least AFAICT currently) to have edit.module implement an alter hook. It's an extension of field formatters' metadata. It's not metadata that edit.module can calculate in any way. It's something that field formatter developers can set for their formatters.
Besides, AFAICT the end result would be the same, no? Edit module would then still alter field formatters' metadata, it'd become part of the field formatter metadata.
More generally, isn't it a valid use case for a module to extend a given plugin type in a certain way, so that more metadata would be needed? Then what would be the recommended course of action in those cases?
Comment #8
tim.plunkettI think you misunderstood my request.
This has nothing to do with moving the 'edit' key out of individual field formatter implementations.
It is about moving the documentation for the 'edit' key out of the new @FieldFormatter annotation class, and documenting it with the module who will use it.
See #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation for relevant discussion.
Something like this, but with actual documentation.
Comment #9
Wim LeersAha! :) Sorry, I completely misunderstood. I thought you were referring to
hook_field_formatter_info_alter()
…It feels bizarre that we can use
hook_entity_info()
to document additional plugin metadata keys though. I suppose it is because a field formatter is now a config entity?Instead of providing ready-to-go docs for in the patch, I'll just explain what happens here, because I don't see a similar example.
This is what the edit annotation looks like:
So, there's a new key in the annotation called "edit" containing any metadata for the Edit module. Currently, there's only one subkey for that, called "editor", which indicates which in-place editor should be used. The default is the 'form' in-place editor (which in turn uses the configured widget), which works for every Field API field type because each field type must have a widget. It's a less-than-stellar experience though, because it less direct than it could be. The 'direct' in-place editor (which is poorly named) allows direct in-place editing of simple textual fields. Hence we configure that as the default for textual fields.
I.e., this is how Edit module determines which in-place editor to use:
Sorry for not providing a copy/pasteable chunk of text. Hope this helps.
Comment #10
tim.plunkettFieldFormatter is a plugin type, not a config entity.
When a module other than Field.module wants to add to the annotation, it needs to document its keys.
Just because Edit.module is in core doesn't mean it can attach itself directly to the FieldFormatter class itself.
Comment #11
Wim LeersRight. Well, as you can see, I'm confused.
I'm not advocating for Edit module to be treated specially.
I just don't see how implementing
edit_entity_info()
allows us to extend the docs of\Drupal\field\Annotation\FieldFormatter
.Comment #12
tim.plunkettOh. Duhh. My apologies, I see why you were confused now!
The idea from the other issue was about @EntityType, so we chose to document new keys on the info/alter hook. This is @FieldFormatter, so hook_field_formatter_info_alter() would be the correct place.
I'm afraid your explanation isn't clear enough to me for me to write up my own docs, but I'd appreciate it if you could just write them out as docs.
Comment #13
dawehnerCan we add a todo to use ContainerFactory or similar pattern on the longrun?
Comment #14
tim.plunkettIs our plan to always use ContainerFactory?
Only two formatters use the container, and both for
\Drupal::token()
:\Drupal\link\Plugin\field\formatter\LinkSeparateFormatter
\Drupal\link\Plugin\field\formatter\LinkFormatter
Rerolling.
Comment #15
dawehnerI guess we will use the same pattern as we do with controllers (make it optional to implement it).
Comment #16
swentel CreditAttribution: swentel commentedSo the problem is that hook_field_formatter_info_alter() is actually an existing hook, so this can't work for the annotation right, or am I missing something completely ?
Comment #17
swentel CreditAttribution: swentel commentedThis should do it - I think.
Comment #19
Wim Leers#17: field-formatter-1985344-17.patch queued for re-testing.
Comment #21
swentel CreditAttribution: swentel commentedOh man, hit me.
Comment #22
dawehnerComment #23
swentel CreditAttribution: swentel commentedreroll
Comment #24
swentel CreditAttribution: swentel commentedStupid doc typo.
Comment #25
yched CreditAttribution: yched commented@tim.plunkett:
Wasn't there also a movement to standardize "discovered" folders to mymodule/lib/.../Plugins/[TheNameOfTheAnnotationClass] ?
(this is not meant to push back from RTBC, just to know where we stand on that topic)
Comment #27
swentel CreditAttribution: swentel commented#24: field-formatter-1985344-24.patch queued for re-testing.
Comment #28
swentel CreditAttribution: swentel commentedRandom failure
Comment #29
alexpottCommitted 984940b and pushed to 8.x. Thanks!
I'm sure that the change notice for field formatters as plugins needs updating... https://drupal.org/node/1805846
Comment #30
swentel CreditAttribution: swentel commentedYes indeed, added to the change notice. Thanks!
Comment #32
xjmUntagging. Please remove the tag when the change notification task is completed.