Convert system_element_info() to Element classes

Looking to tackle URL in another issue

Here's a start from the meta.

This one will be big. I'm going to start on it, @larowlan I'll skip URL for now, just crosslink the issue when you can.

URL to be handled in #2321745: Add #type => 'path' that accepts path but optionally stores URL object or route name and parameters

Had to take a break, but here's what I have so far.

Now all of them are in.

Guess weight is used in a few places.

If this one fails I'll take it to a test-only issue, but I feel better about this one.

Color and Views UI were calling process and value callbacks directly.

Okay, now added as many docblocks as I could pull out of FAPI docs.
I'll ping @jhodgdon for her suggestion on what to do for the rest.

The following classes need one line docs.

core/lib/Drupal/Core/Render/Element/Color.php
core/lib/Drupal/Core/Render/Element/ConditionalFormElement.php
core/lib/Drupal/Core/Render/Element/Details.php
core/lib/Drupal/Core/Render/Element/Dropbutton.php
core/lib/Drupal/Core/Render/Element/Email.php
core/lib/Drupal/Core/Render/Element/HtmlTag.php
core/lib/Drupal/Core/Render/Element/InlineTemplate.php
core/lib/Drupal/Core/Render/Element/Label.php
core/lib/Drupal/Core/Render/Element/LanguageSelect.php
core/lib/Drupal/Core/Render/Element/Operations.php
core/lib/Drupal/Core/Render/Element/Number.php
core/lib/Drupal/Core/Render/Element/Page.php
core/lib/Drupal/Core/Render/Element/Range.php
core/lib/Drupal/Core/Render/Element/Scripts.php
core/lib/Drupal/Core/Render/Element/Search.php
core/lib/Drupal/Core/Render/Element/Styles.php
core/lib/Drupal/Core/Render/Element/Table.php
core/lib/Drupal/Core/Render/Element/Tel.php
core/lib/Drupal/Core/Render/Element/Url.php
core/lib/Drupal/Core/Render/Element/Value.php

I'll make a patch for the one-liners, coming shortly...

Here's a new patch, with some additional documentation.

A few notes:

a) I'm not sure about the name of the ConditionalFormElement class -- this is a base class that is used for checkboxes and radios. Why is it being called "Conditional"? What is Conditional about it? I don't really understand the name. Maybe call it OptionGroupFormElement or GroupOfOptions or OptionGroup or ???

b) I'm a bit confused by the Table class. It extends FormElement -- I think this is because TableSelect needs to extend this class, and TableSelect is a form element. But Table itself is not a form element -- it is just a generic table HTML element. Perhaps the shared methods should be put on a Trait instead or something? Because as it is... it's a bit weird for a render array that needs a table to add something that's a FormElement to the array. I don't think we would want the generic Form Element processing to be put on Tables either?

Anyway... Those two oddities aside, here's a patch that adds/revises a bunch of documentation. I tried to make the one-line docs as consistent as possible.

Thanks @jhodgdon! Looking at ConditionalFormElement closer, it is actually about composite form elements. Renamed, and converted to a trait so that other elements can use it more easily without having to worry about inheritance.

That should be it?!

Do you have any thoughts on Table (see comment #19-b)?

We're not changing how Form API or drupal_render() works there, so we only need to change #type 'table' if/when those are changing. Until then, #type 'table' should work as FormElement within & without form context, so let's keep it that way.

It needs to be a FormElement, due to its optional support for #tableselect, which requires and only works in a form context. However, even in that case, it's still a table. That's what ultimately matters, because each element #type is supposed to represent a discrete interface concept. The end result is a <table>.

You're choosing to sacrifice API consistency for a minor DX win. Not a very good trade-off.

I'm not able to see where API consistency is sacrified? Allowing an element type to work in both contexts has little to do with inconsistency. Can you provide examples?

All it does is to satisfy the assumed interfaces of two integration points, so as to provide a unified API and structure for working with tables, which vastly improves DX. (#tableselect is just a Boolean.)

Different topic: I didn't expect these child issues to get picked up this quickly… — As mentioned in the original/first spin-off issue #2325049-3: Convert datetime_element_info() to Element classes:

I'm concerned about all the different method names. processWeight() should be just process(), ditto for other callback methods.

We don't need suffixes in the method names, because these are independent classes; they're scoped by themselves already. Weight::processWeight() duplicates itself in its very own method name and harms DX, because method names are always different.

I'm not renaming any of the methods. a) there's no reason to and nothing gained b) it wouldn't work.

Take Tableselect extends Table

Tableselect has two #process callbacks. processTableSelect and processTable. If both were named process, it could not have both.

@jhodgdon: #type 'vertical_tabs' is also a FormElement, but has been designed to work as a pure render element, outside of a form context, too. This was explicitly resolved in response to a bug report (IIRC, for D7 already).

Likewise, #type 'fieldset' should also work without a form, because it's a native HTML element. The HTML standard only specifies that it MAY appear in a form context, but that is not a requirement. This is either the case in HEAD already or the bug report is not resolved yet.

There are some more #types in this category. #type 'table' is in that category; it's not an exception.

@tim.plunkett: If there was an interface, they would be named the same, right? Why would we not want to work towards interfaces?

Note that the separate #type 'tableselect' (and thus processTableSelect()) should theoretically be obsolete in HEAD, since all tables should hopefully be converted to #type 'table' by now.

Regardless of that, if Tableselect extends Table, then Tableselect::preRender() can call its parent Table::preRender(), as in any other class inheritance model. Why would we want to specify two different callbacks to perform a single operation? Previously, we had to, because there's no inheritance in functions.

Not all form or render elements have a #pre_render or #process. And many of them have multiple. How would you have an interface for that?

If you want to bikeshed a brand new way of doing things, please take it to another issue.

As already discussed on the original issue that introduced the initial concept, #2305839: Convert hook_element_info() to annotated classes

We have a FormElementInterface already. Apparently, valueCallback() is standardized for all element types.

We have FormElement and RenderElement base classes already. They can implement the respective interface with empty methods by default.

Doing so allows us to properly document each of these callbacks. Not doing so doesn't.

Among other benefits, ending up with a consistent and clearly defined API for element types has been my understanding of #2305839. It was deliberately committed in an incomplete state - completing the interfaces was explicitly deferred to follow-up issues. Are you suggesting that we first convert all the things and change all of them once more in a separate issue? The process is no longer clear to me.

There can be only one #value_callback, and yes, all forms elements have one (because they need to deal with #input)

#element_validate, #pre_render, and #process are arrays of callables.

There was never any plan to introduce an interface for those.

I took a careful look through the patch.

a) It looks like you've moved the FormElement::processAjaxForm() method to RenderElement, so probably this needs an update?

 * @deprecated Use \Drupal\Core\Render\Element\FormElement::processAjaxForm().
  */
-function ajax_process_form($element, FormStateInterface $form_state, &$complete_form) {
+function ajax_process_form(&$element, FormStateInterface $form_state, &$complete_form) {
   return Element\FormElement::processAjaxForm($element, $form_state, $complete_form);
 }

Also since it is on RenderElement, maybe it shouldn't have "Form" in the method name and documentation?

b) Since I don't think we're going to resolve the Table question and I don't want to hold this up, I think (a) should be fixed and then I'm +1 for RTBC. I could complain about documentation of some of the methods, but it was copied from lame documentation on the same functions... we should just get this in and then refine if necessary.

#process callbacks only run in a form context, they are called by FormBuilder. But yes, it was moved up to RenderElement, because it is also used by render elements that need special behavior when used inside a form.

The Table nonsense is very frustrating, but it looks like @sun designed '#type' => 'table' to disregard all of this from the very beginning, and fixing that is out of scope.

Looks good then! Let's get this in.

Thanks!

This patch missed a lot of references to deleted functions. Mostly in comments, but form_validate_number() is also still explicitly used as a callback in NumericItemBase (apparently untested).

The attached patch would fix all of those which I could spot (using some grepping). I'm just not sure, do we have a standard for referencing methods of the same class in comments? I guess using the FQN would be overkill in this scenario? I just used the method name itself in this patch, but that should be easy enough to fix (e.g., by more grepping).

@drunken monkey since this issue is fixed and a patch has already been committed, I *think* you'll get more attention by filing a follow-up issue referencing this one.