Convert hook_element_info() to annotated classes

Thanks a lot for kick-starting this, @tim.plunkett! :-)

1. 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.

2. 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.)

3. 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.

4. Can we change the method name from getInfo() into getDefaults() or getDefaultProperties()?

   Because that's what it is called internally; cf. drupal_render():

     if (isset($elements['#type']) && empty($elements['#defaults_loaded'])) {
       $elements += element_info($elements['#type']);
     }
   

Agree 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:

+ * Element info plugins allow modules to declare their own Form API element
+ * types and specify their default values.

Let's call them Form and Render API types.

b) Annotation class:

 * This hook allows modules to declare their own form element types and to

Um, hook? The rest of the paragraph also talks about hooks.

c) Manager class:

  protected function buildInfo() {
+    $info = $this->moduleHandler->invokeAll('element_info');

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

 * @ThisPluginType(
 *   id => 'the_id_value'
 * )

right? Maybe this is a shorthand? I wasn't familiar with it, and it is not in the @ingroup plugin_api docs.

e)

/**
+ * Formats a single-line text field.
+ *
+ * @ElementInfo("textfield")
+ */
+class Textfield extends ElementInfoBase {

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.

Raising 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.

Put 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?

Or maybe \Drupal\Core\Installer? [edit: or maybe not, since those elements are needed during non-install time as well]

I'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?

This 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?

So unless we can think of something else that the plugin classes should do

It certainly feels wrong for these procedural functions to be in the plugin

What 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.

unless they are going to be instantiated

The above doesn't make them instantiable, but at least it makes them not totally bare classes.

Static 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....

Converting 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 and FormElement, 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).

I ***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.

Yes, 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() and handleUserInput($input).

1. +++ b/core/lib/Drupal/Core/Render/Element/FormElement.php
   @@ -0,0 +1,155 @@
   +abstract class FormElement extends RenderElement implements FormElementInterface {
   ...
   +  public static function processAutocomplete(&$element, &$form_state, &$complete_form) {
   

   Maybe a dump question but I would have expected this on a textfield.

2. +++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
   @@ -0,0 +1,223 @@
   +class MachineName extends Textfield {
   ...
   +        array($class, 'processAutocomplete'),
   
   +++ b/core/lib/Drupal/Core/Render/Element/Textfield.php
   @@ -0,0 +1,74 @@
   +class Textfield extends FormElement {
   ...
   +        array($class, 'processAutocomplete'),
   

   Seems to proove the previous idea.

3. +++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
   @@ -0,0 +1,97 @@
   +
   +  public function getInfo($type) {
   

   needs docs

4. +++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
   @@ -0,0 +1,97 @@
   +   * @return \Drupal\Core\Render\Element\ElementInterface
   +   */
   +  public function createInstance($plugin_id, array $configuration = array()) {
   +    return parent::createInstance($plugin_id, $configuration);
   +  }
   

   <3

5. +++ b/core/tests/Drupal/Tests/Core/Render/ElementInfoTest.php
   @@ -7,12 +7,11 @@
    class ElementInfoTest extends UnitTestCase {
   @@ -20,11 +19,18 @@ class ElementInfoTest extends UnitTestCase {
   
   @@ -20,11 +19,18 @@ class ElementInfoTest extends UnitTestCase {
      /**
   

   Given that we expand the functionality we should also adapt our test coverage, don't we?

I guess it is a real failure.

#2316941: Use the builder pattern to make it easier to create render arrays might serve as way to document the properties (read magic keys).

I 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.

I 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.

This is ready.

+++ b/core/modules/system/theme.api.php
@@ -214,15 +221,29 @@
+ * @section elements Render elements
+ * Render elements are defined by Drupal core and modules. The primary way to
+ * define a render element is to create a render element plugin. There are
+ * two types of render element plugins:
+ * - Generic elements: Generic render element plugins implement
+ *   \Drupal\Core\Render\Element\ElementInterface, are annotated with
+ *   \Drupal\Core\Render\Annotation\RenderElement annotation, go in plugin
+ *   namespace Element, and generally extend the
+ *   \Drupal\Core\Render\Element\RenderElement base class.
+ * - Form input elements: Render elements representing form input elements
+ *   implement \Drupal\Core\Render\Element\FormElementInterface, are annotated
+ *   with \Drupal\Core\Render\Annotation\FormElement annotation, go in plugin
+ *   namespace Element, and generally extend the
+ *   \Drupal\Core\Render\Element\FormElement base class.
+ * See the @link plugin_api Plugin API topic @endlink for general information
+ * on plugins, and look for classes with the RenderElement or FormElement
+ * annotation to discover what render elements are available.
+ *
+ * Modules can also currently define render elements by implementing
+ * hook_element_info(), although defining a plugin is preferred.
+ * properties. Look through implementations of hook_element_info() to discover
+ * elements defined this way.

Just WOW. Thank you @jhodgdon for explaining this.

Didn't even take ten years to find a sustainable way to document this. Absolutely great.

This 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:

+  public function getInfo() {
+    $class = get_class($this);
...
+      '#pre_render' => array(
+        array($class, 'preRenderTextfield'),
+        array($class, 'preRenderGroup'),
+      ),
+  }

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

Out 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.

What 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.

At 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.

For 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 :-)