Problem/Motivation
#1617948: [policy for now] New standard for documenting form/render elements and properties explains the problem space.
Proposed resolution
This will introduce a new ElementInfo annotation, and use that to replace hook_element_info().
We will NOT provide a plugin-level alter, since ElementInfo will only contain the plugin ID in the annotation.
The actual element info will be in a method on the class.
The existing alter hook will remain, running on the collected info.
Remaining tasks
Decide on more advanced ways to document the properties used by the elements (or punt that to another issue?)
User interface changes
N/A
API changes
No API change yet, but Drupal\*\ElementInfo\* will act as a replacement for hook_element_info().
Comment | File | Size | Author |
---|---|---|---|
#53 | element_info-2305839-53.patch | 72.5 KB | jhodgdon |
#53 | interdiff.txt | 25.2 KB | jhodgdon |
Comments
Comment #1
tim.plunkettComment #5
sunThanks a lot for kick-starting this, @tim.plunkett! :-)
Can we globally rename "ElementInfo" into "RenderElement" in classes and annotations?
→ "Element" is ambiguous/unclear, and "Info" has no meaning. It's about render elements.
Does the Annotation architecture support "sub-annotations" that are more specialized variants of a parent/generic annotation?
→ If so, could we have both @RenderElement and @FormElement?
(FormElement is more specialized than a generic RenderElement.)
Can we change the subdirectory in which plugins are discovered into just
./Element/
?I'm aware that this question/suggestion slightly conflicts with (1), but given (2), the directory would hold both RenderElement and FormElement plugins.
Can we change the method name from
getInfo()
intogetDefaults()
orgetDefaultProperties()
?Because that's what it is called internally; cf.
drupal_render()
:Comment #6
jhodgdonAgree with sun's review, mostly.
In point 1, that would also apply to the service name.
In point 2, I think there are really 3 types of elements, as noted on the parent issue:
- render elements
- form input elements
- other form-related elements
Some additional comments:
a) The annotation class:
Let's call them Form and Render API types.
b) Annotation class:
Um, hook? The rest of the paragraph also talks about hooks.
c) Manager class:
Might want a @todo here to say we'll remove this when it's all converted? Or is that the plan?
d) When I got to the TextField type and its annotation, I went back to the Annotation class. It doesn't tell me that in an actual annotation, all I need to do is put in the machine name of the type being defined. Is that obvious? Maybe document it? Because I think for most plugins, you would do something like
right? Maybe this is a shorthand? I wasn't familiar with it, and it is not in the @ingroup plugin_api docs.
e)
I think maybe for the first line here, I would say "Provides the 'textfield' form input element." ?
f) If hook_element_info() is deprecated, we should have an @deprecated line added to its documentation. If we're going to try to keep both hook and plugin, we should at least add to the info hook some information about the new plugin type, eventually.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedRaising to Major to match #1617948: [policy for now] New standard for documenting form/render elements and properties. Please correct me if that isn't correct.
Comment #8
tim.plunkettWell, before I dive in to all that, we need to get it to install first.
The list of namespaces handed to plugin discovery includes NO modules:
$this->getConfigStorage()->read('core.extension')
is an empty array in DrupalKernel.However, $this->moduleHandler->getModuleList() includes 'system', so the invokeAll('element_info') works.
Advice welcome.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedPut the ones needed by the installer into \Drupal\Core\Render and/or \Drupal\Core\Form? Except some of them reference procedural functions, which I guess isn't ok?
Comment #10
effulgentsia CreditAttribution: effulgentsia commentedOr maybe \Drupal\Core\Installer? [edit: or maybe not, since those elements are needed during non-install time as well]
Comment #11
tim.plunkettWell that's still a hack right? I mean, I'm moving them out of system_element_info, and system.module exists at this point.
Isn't this just uncovering another bug with the config storage not being updated soon enough?
Comment #12
jhodgdonI'm not sure about the config bug, but it doesn't seem terrible to me to have the "core" elements in the "core" namespace. We have a lot of other stuff in the "core" namespace. The basic elements are not really part of the "System Module" per se -- they're just in system_element_info() currently because it is a hook currently and hooks must live in modules.
Right?
Comment #13
tim.plunkettAs @effulgentsia points out in #9, almost every single element has at least one procedural function accompanying it. It will likely still work fine for those to be called, but is that acceptable to rely on?
Comment #14
tim.plunkett#5.1 Done
#5.2 Done
#5.3 Done
#5.4 Not done yet, because though you're right about the calling code using these as "defaults", the service is called element_info, the alter hook that isn't going away is hook_element_info_alter, and we're migrating from hook_element_info. I think this needs further thought.
#6.a I didn't write any docs yet for FormElement, because I'm not sure how to best split the docs between that an RenderElement
#6.b I didn't fix that, for the same reason.
#6.c Added todo
#6.d This is because we extend @PluginID, not @Plugin. I think @jhodgdon was opening another issue about this
#6.e The string I have is straight from the FAPI docs, I'm not sure what to do with that. Your suggestion *does* sound better.
#9, It certainly feels wrong for these procedural functions to be in the plugin, but it does work, and shouldn't hold us up.
Comment #16
tim.plunkettUgh, phpstorm only did half of the rename right.
Comment #18
tim.plunkettUGH. Filing an issue now.
Comment #20
tim.plunkettUgh I put it in the wrong dir.
Opened #2309889: DrupalKernel requires a Drupal\Core\*\Plugin directory for annotated classes to be discovered.
Comment #21
jhodgdonThis looks conceptually OK to me... I guess. Obviously some docs things to fix up (for one thing I think some of the @see links are pointing to classes that moved since your original patch?), and I think I would rather call the form input element plugin FormInputElement rather than FormElement, but ... bikeshed.
So... There is one kind of overriding question that comes up here, which I discussed briefly in IRC with Tim just now.
These are kind of poor lame little fake plugins. All they have is a (a) the potential for some documentation and (b) a getInfo() method, and the getInfo() just returns what hook_element_info() used to return. The classes don't do anything else, and no method other than getInfo() is ever called on the plugin objects. ... This seems like a better use case for hooks than for plugins, to me.
So unless we can think of something else that the plugin classes should do, and unless they are going to be instantiated during processing of forms or render arrays, instead of only during the "figure out what element definitions exist and cache them" stage, I think we'd be better off not converting them to plugins.
Thoughts? Can we think of anything more that these element plugins should be doing, and that we can actually accomplish?
Comment #22
effulgentsia CreditAttribution: effulgentsia commentedWhat if we fix both problems by moving those procedural functions to static methods in the plugin classes. E.g., 'form_pre_render_textfield' -> '\Drupal\Core\Render\Element\TextField::preRender'? We'd need to find appropriate places for shared #process and #pre_render callbacks, but I think we could do that.
The above doesn't make them instantiable, but at least it makes them not totally bare classes.
Comment #23
jhodgdonStatic methods would seem to make sense... The common ones could go on the base classes (for Render and FormInput elements); specific ones could go on the specific classes for those elements.
I'm not sure that this would necessarily make the plugins less lame though, since still the main functionality of each plugin would be to return the info array.... which again still sounds like it's a thin wrapper around what is essentially a hook functionality.
Now if the plugin classes were actually instantiated and used during rendering, that would be interesting....
Comment #24
sunConverting the procedural functions into static methods on the plugin classes definitely makes sense. Doesn't necessarily need to happen as part of this issue though.
The element type specific functions are callbacks, not hooks. Callbacks are a 1:1 mechanism. Hooks are a 1:n mechanism.
As long as there is no state, static class methods are perfectly fine. It would be pointless to introduce state where no state is supported. Introducing state at the render/form element level is D9 material at this point.
For D8, we could investigate whether it would make sense to introduce abstract base classes for both
RenderElement
andFormElement
, which would only consist of empty implementations of (all available) callback methods. Possibly plus interface(s) [although an interface enforces all methods onto a class, which doesn't really make sense for optional callbacks]. This might help to resolve the problem of documenting the possible callback methods (and standardizing their names).Comment #25
tim.plunkettI won't know until I convert more, I don't know which methods should go on which class.
But here's some examples.
Comment #27
tim.plunkettWhoops, didn't notice that only *some* of the process callbacks used &$element as a reference.
Ironed out some other weirdness, and converted one more form element (machine_name which extends textfield) and one render element (link).
Still need docs for the FormElement base class and annotation class.
What do you think about the rest of the conversions? Should we just get this in as-is and convert the rest per module?
Comment #29
tim.plunkettGreat copy/paste mistake, or GREATEST copy/paste mistake?
Comment #30
jhodgdonI ***love*** the direction this is going!
Minor detail: I'm not sure about the choice of @deprecated vs. outright removal. It looks like most of the functions were marked as deprecated and one was removed... not sure which way we're leaning in Core at this point, but we should probably be consistent.
Comment #31
tim.plunkettYes, I was a little overzealous in removing that.
I resolved the @todos, and fixed up some bad @see statements since the last round of renaming.
IMO, this should be ready to go in, #2311393: Remove hook_element_info() and all references to it will handle the remaining conversions.
Comment #32
tim.plunkettSo! I had an idea for another usage of these classes. Form elements can specify a value_callback, but if they don't it tries to use form_type_TYPE_value. That can be another method on the class.
Comment #33
sunYes, totally. That's exactly what I had in mind, too.
#1013722: Drop #value_callback auto-suggestions, declare them explicitly like everyone else
#946570: Use and call explicit #value_callback instead of magic form_type_TYPE_value()
#447930: #value_callback documentation and signature (D7)
The moment we do that, however, should be accompanied by a
FormElementInterface
and/or base class though; it's one of the (very few) callbacks/methods that are sorta enforced on all form element #types (accepting #input) — only a few types are able to work with the default (and loose) input string == value string handling.Moreover, the current #value_callback is overloaded; it gets called if there's user input and if there is none. The latter is reflected as
$input === FALSE
in all implementations. So we're really talking about two separate methods,getDefaultValue()
andhandleUserInput($input)
.Comment #34
tim.plunkettWe have a base class, I considered adding an interface but figured I'd wait for feedback first.
Yep, I was just staring at this for the past 30 minutes and considering what to do. I'm very glad you agree they should be split up.
Comment #36
tim.plunkettHmmm I looked at splitting #value_callback in two, but it doesn't really seem feasible until the rest of the elements are converted. Also, that would mean some judgement calls when splitting up the callback, as opposed to a straight port here...
I'm going to hold off on that. Here's an interface, and I had to convert the color palette code since it used textfield code.
Comment #38
tim.plunkettComment #40
tim.plunkettUghhh. Reverting the color.module changes, that can come later.
Comment #41
dawehnerMaybe a dump question but I would have expected this on a textfield.
Seems to proove the previous idea.
needs docs
<3
Given that we expand the functionality we should also adapt our test coverage, don't we?
Comment #42
tim.plunkett#41.1
#41.2
'tel', 'email', 'url', and 'search' also use processAutocomplete, and do not really reuse anything else from Textfield. Though we visually display them that way... I guess we could force it.
#41.3
#41.5 Done
Comment #43
tim.plunkettRerolled for #2225353: Convert $form_state to an object and provide methods like setError().
Comment #47
jhodgdonI guess it is a real failure.
Comment #48
tim.plunkettYep, sloppy reroll.
Comment #49
larowlan#2316941: Use the builder pattern to make it easier to create render arrays might serve as way to document the properties (read magic keys).
Comment #50
jhodgdonI asked on that other issue whether the two proposed classes (the plugins here and the "helpers" there) could be combined.
Even if they can't for some reason, maybe we could put the properties of the render element into the plugin class as member variables, and document them there? I guess it would add a tiny bit of storage and overhead when the plugin is instantiated, which I think is only when the container is being built, but ... we're talking about a few strings and empty arrays, right? That would seem like a fairly ideal way to document properties, and they'd also be automatically inherited... ?? maybe.
Comment #51
tim.plunkettThis interface can be expanded iteratively. We can get this in first, and follow-up in that issue.
Reroll, no changes.
Comment #52
tim.plunkettI think this has been taken as far as it should go, and the rest should be follow-ups.
Assigning to @jhodgdon for her sign-off (or any needed tweaks).
Comment #53
jhodgdonI think this looks great!
I made a "few" documentation tweaks. I am very happy with this and am +1 on RTBC. I reviewed the code, idea, and documentation. Someone should probably at least review the changes I made to the docs in this interdiff... but I think this is ready to go in.
Comment #54
jhodgdonComment #55
jibranThis is ready.
Just WOW. Thank you @jhodgdon for explaining this.
Comment #56
tim.plunkett+1, thanks!
Comment #57
chx CreditAttribution: chx commentedDidn't even take ten years to find a sustainable way to document this. Absolutely great.
Comment #58
webchickThis patch is fantastic.
I had both chx and timplunkett walk me through this patch. I did have some mild questions/concerns, like why we chose to lump in a few renames here that make the patch bigger, and around the DX/verbosity of doing:
But that's fairly minor. The number of developers out there writing hook_element_info() is like 1/100000ths the number of developers actually using form/render elements in their code, and for them this change has zero impact. But it has the side benefit of making the code easier to understand, and easier to document. WIN!
Committed and pushed to 8.x. Thanks SO much! :D
Comment #60
XanoOut of curiosity, why are these plugins not located under a
\Plugin
namespace, and why do the annotations not allow additional properties? Especially the latter could be limiting for contrib.Comment #61
jhodgdonWhat additional properties are you talking about? The plugin has a method to provide defaults for the render array properties, and the plugin class can have class member variables... I don't think we need to provide any additional annotation stuff? Can't see why.
Comment #62
XanoAt this moment we don't, but if contrib, for instance, wants to add extra metadata to element definitions, do annotations with only one property allow that? They're not keyed arrays in this particular case.
Comment #63
tim.plunkett@Xano If you want to open a general issue about removing the PluginID annotation class and always using an array, that's fine.
But the element info service has no need for those keys, and adding them in contrib won't do anything.
Comment #64
yched CreditAttribution: yched commentedFor anyone interested, #2328061: Move datetime's FormElement #type classes in Core would be a great place to leverage this, and would help untangle #2226493: Apply formatters and widgets to Node base fields :-)