Move node base widgets to the top level of the form

1: node-base-fields-top-form-elements.patch queued for re-testing.

1. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
   @@ -184,12 +184,12 @@ public function form(array $form, array &$form_state) {
        );
   ...
   +    $form['date'] = array(
          '#type' => 'textfield',
          '#title' => t('Authored on'),
          '#maxlength' => 25,
          '#description' => t('Format: %time. The date format is YYYY-MM-DD and %timezone is the time zone offset from UTC. Leave blank to use the time of form submission.', array('%time' => !empty($node->date) ? date_format(date_create($node->date), 'Y-m-d H:i:s O') : format_date($node->getCreatedTime(), 'custom', 'Y-m-d H:i:s O'), '%timezone' => !empty($node->date) ? date_format(date_create($node->date), 'O') : format_date($node->getCreatedTime(), 'custom', 'O'))),
   -      '#default_value' => !empty($node->date) ? $node->date : '',
   +      '#default_value' => !empty($node->date) ? (string) $node->date : '',
   

   The reason this is weird is because you haven't converted datetime_form_node_form_alter() yet. $node->date is an object if it is enabled and it then changes the #type of this.

2. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
   @@ -208,22 +208,64 @@ public function form(array $form, array &$form_state) {
   +  public function preRenderStructure($form) {
   +    // Promotion options.
   +    if (isset($form['promote'])) {
   +      $form['options']['promote'] = $form['promote'];
   +      unset($form['promote']);
   +    }
   

   Unsetting a form element to hide is wrong. What shold be done is setting #access.

   Not sure if we want to make the code complicated by supporting that things can be unset.

let's give it a try, field renaming and some beauty fixes are still needed

Lik berdir said, the unsetting is really something we shouldn't be doing here.

What if we explore whether form_process_group() can also work on the details type, that way elements can tell themselves in which group they belong ?

Yeah, just being able to set a property to define where it should be moved to would be great.

Much saner solution. We could add this 'group' rendering to any element basically. This gives us more or less programmatically field group functionality.

Rendering and submission of node works fine.

And the interdiff. Forgot to update the date element, but want to hear first whether this approach is what we want or not.

@Gabor's code was not hiding an element but moving it to a different place in the $form structure - thus unset() rather than #access IMO.

But anyway, +(a lot) for a form_process_group-like approach :)

What date module is doing is really weird BTW. Just enabling a field-type module switches in a different type for an existing form element :-/. Anyone knows whether we have an issue to clean that up ?

Don't think there's an issue for that. I think that switch happened after the decoupling of datetime and node module. They used to be dependant on each other.

To me it feels like datetime module should only be the field, all other stuff should just move to core.

We did that to avoid a hardcoded dependency on datetime from node. No issue to change it, but there *is* an issue to fix it, because it's completely broken and untested right now.

Maybe we can find a nicer solution through widgets.

Thanks for the inputs.
Here's a patch what takes care of renaming the items, as Gabor requested in the initial issue description, to match the base fields, also it uses the #group for relocating, taking care of datetime too.
It needs work as it fails some node tests even at localhost.

I double checked, Drupal\content_translation\ContentTranslationController\addTranslatabilityClue() is executed before the group processing kicks in, so it should be fine.

the previous patch was with unix coloring codes...

@Berdir, @swentel : Yeah, in the end we'd want the date to be simply handled by a widget. The problem is then the different datetime / timestamp field types.

For testbot.

If someone wants to proceed, here are some hints:
the tests fail, because the promote bit's default value is not handled properly when the class in AggregatorBase.php creates test nodes, nodes are unpromoted. It cannot be reproduced by creating nodes at the UI however...
So couple of tests fail because of this, i started to debug, what i see so far is that it goes wrong at ContentEntityFormController::buildEntity(), where promote 0 is set. Not sure why at the moment, as form_state contains good value.

the tests won't pass, so NW still
rerolled because of latest 8.x commits, to make it apply cleanly

The reason for some of those test fails is now that the process stuff happens after the form parents have been set, so the form element is no longer date[date] but just date.

I'd be ok with saying that is by design now, then we simply need to fix those tests :)

Yup, it's totally by design. The #process approach means form values are now at the top level of $form_state['values'], independently of presentational layout.

That way it's the same for fields handled by widgets and fields handled by custom code. Hopefully it should also simplify some code at submit time ?

In case of date/author not really, because both are a virtual widget (no direct mapping to a field but needs prepare and submit special handling anyway). Maybe we can clean but probably not here :)

But yes, I agree, makes sense to me.

So, the problem is that the form elements themself don't have #access definitions anymore. So when displaying the form, they're kind of visible but are then moved to the group where they aren't displayed. Then the non-submitted value is preferred over the default value, because the form system assumes they're visible, as it knows nothing about pre_render stuff.

Also removed a bunch of unecessary code that confused me when debugging, as we seem to be cleaning up that form anyway ;)

The only thing that I'm not sure about is if someone is still relying on those old value properties in the form, removing it anyway because I want to know...

Hm. A few thoughts on this:

1. Form element keys/names are seemingly renamed to match their base field names + entity object location (top-level).

   It is not clear to me whether this is a hidden attempt at establishing a 1:1 mapping and dependency between form element names and entity/domain object data models.

   If this is done for convenience reasons only, then that's fine. But if this turns into a requirement, then it is not.

   The form structure and presentation is decoupled from the internal data model. The default assumption for user input in a form is that it is not compatible with the data model and has to be mapped and processed and transformed into the expected data model location and format. That's the job of #value_callback.

   That not only applies to the data model, it also applies to the internal form structure: There is no enforced constraint for the internal location of a form element to be in the same location as the form input value. That's the job of #parents (+ #name + #tree for that matter).

   So if this is an attempt to establish a 1:1 binding, then I cannot support that for architectural reasons and we should seek for alternative solutions.

2. All form elements are moved to the top-level and there is no more form element hierarchy in the $form structure.

   In other words, the form constructor, #process, and hook_form_alter() are operating on a flat $form array and need to manually decipher the final hierarchy of form elements by introspecting all #group properties that possibly exist (anywhere) in the form.

   It appears that this concept was originally introduced in #1838114: Change node form’s vertical tabs into details elements and further advanced on in #1856178: '#group' Form API property only works with <details> elements.

   I'm very concerned about the DX of constructing, altering, #after_build'ing, #pre_render'ing, theming of forms, because the #group mechanism causes the $form structure to highly vary and be completely different, depending on in which hook/callback facility you're introspecting and accessing it. — Adjusting/enhancing/manipulating forms is still the first and foremost use-case of developers working on custom sites.

   We essentially turned the $form array into a flat, one-dimensional array and moved the entire concept of any kind of hierarchy into #group processing.

   $form->addElement('title', array(...));
   $form->addElement('body', array(...));
   $form->addElement('created', array(...));
   $form->addElement('submit', array(...));
   // It's flat until rendered.
   $form->render();
   

   → The entire hierarchical recursion concept of FormBuilder::doBuildForm() as well as drupal_render() is obsolete with that, because we're operating on a flat array; there are no element children anymore.

   I guess, changing that concept is fine, but what really concerns me is the inconsistency. The node form is the one and only form that implements this concept. All other forms are still using legacy/hierarchical definitions.

   It's great that we're polishing the node form more and more to get it right, but I think we should be seriously concerned about the inconsistent state of the rest of the system.

   → It would be a very good idea to start an effort to convert all forms throughout core to the new concept, so as to ensure and guarantee a good and consistent DX.

3. Each time I'm looking at the node form code and patches like this here, I have a hard time in figuring out where the group elements themselves are actually defined and coming from.

   I think we should also think about that.

4. This patch adds #process and #pre_render callbacks to additional form element #type definitions.

   Given that this concept is turned into a de-facto standard approach and algorithm to handle grouping of form elements, it does not make sense to leave that processing to some fragile callbacks that may or may not be defined.

   → The form_process_group() and form_pre_render_group() callbacks should be removed and the concept should be refactored directly into FormBuilder.

I'm aware that parts of this review are beyond the scope of this issue. But given what we're doing here, I have to raise awareness for the architectural big picture — especially since we're shooting for a release at some point.

My impression is that we have good ideas and we're making good progress on the silo that is node form, but I guess what I'm missing is an architectural plan.

I'm not really able to see how we can release D8 in the current, very inconsistent state.

it is not clear to me whether this is a hidden attempt at establishing a 1:1 mapping and dependency between form element names and entity/domain object data models.

That's what it is, it's not hidden :-). Form elements for *content entity fields* need to be placed at the root of the $form, notwithstanding presentational considerations.
That's :
- if they want to be translatable
- if they want to use Field Widgets
That's the cost of providing generic features - cannot be done if the elements can live at any random place in the $form or $form_state['values'] structure.

That's nothing new either, CCK D6 / core D7 field widgets have always been generated at the top level of the form, with field_groups or similar doing presentational reshuffling only at render time. This never seemed to bug anyone ?
It is also much more friendly for _alter() code, since you don't need to wonder whether some module has already moved the element you're looking for at some other place, depeding on some random configuration.

That's nothing new either, CCK D6 / core D7 field widgets have always been generated at the top level of the form, with field_groups or similar doing presentational reshuffling only at render time. This never seemed to bug anyone ?

There are different use-cases here that need to be consider. If I just want to change the #default_value or the #description of a form element this patch makes that easier because I can just blindly to that in $form[$field_name]['#foo'] and don't need to bother with possible nesting.

Another use-case, however, is changing the *output* structure of the form, i.e. moving form elements into different groups, adding form elements into existing groups, perhaps nesting groups into each other, etc. With fieldgroup.module in conrib this is currently a non-trivial task that requires deep knowledge about the different structures and array keys that fieldgroup module uses. It's not as easy as simply setting/altering $form[$field_name]['#group'] or anything similar.

I have not reviewed the patch in detail but we should consider the latter use-case as well, and make that as easy as possible.

In other words: Yes that has been bugging me for a long time!

Looks pretty good. Note that the created/date field on nodes currently has various bugs and almost zero test coverage, see #2031183: Improve test coverage for node authored on widget. We probably don't need to worry about it here and just update that issue once this is in, but I wanted to mention it in case someone does manual tests.

@@ -177,7 +164,7 @@ public function form(array $form, array &$form_state) {
-    $form['author']['name'] = array(
+    $form['uid'] = array(

@@ -316,11 +311,11 @@ public function validate(array $form, array &$form_state) {
-      $this->setFormError('name', $form_state, $this->t('The username %name does not exist.', array('%name' => $form_state['values']['name'])));
+      $this->setFormError('uid', $form_state, $this->t('The username %name does not exist.', array('%name' => $form_state['values']['uid'])));

@@ -423,15 +418,15 @@ public function buildEntity(array $form, array &$form_state) {
-    if (!empty($form_state['values']['name']) && $account = user_load_by_name($form_state['values']['name'])) {
+    if (!empty($form_state['values']['uid']) && $account = user_load_by_name($form_state['values']['uid'])) {

is uid really the uid, a number, or a name?

if we are using uid now, then the message might change to:
The user %uid does not exist.
or the %name arg should look up the name from the uid.

-
woah. it's using user_load_by_name... and passing in from the 'uid' element?

(#48 2. bit related)

maybe we should keep it as 'uid'?

So @Berdir said in irc, that it's part of the point of this issue:

Move all base fields to the top level and have them equal names to the base fields.

How do I look up the name of the base field for the 'name' 'uid' in node form?

I see

    $form['uid'] = array(
      '#type' => 'textfield',
      '#title' => t('Authored by'),
      '#maxlength' => 60,
      '#autocomplete_route_name' => 'user.autocomplete',
      '#default_value' => $node->getOwnerId()? $node->getOwner()->getUsername() : '',
      '#weight' => -1,
      '#description' => t('Leave blank for %anonymous.', array('%anonymous' => $user_config->get('anonymous'))),
      '#group' => 'author',
      '#access' => user_access('administer nodes'),
    );

in NodeFormController
But not sure where to look for the "base" of that.
Didn't see anything promising in EntityFormController.

well node_schema() in node.install isn't the place either.

In Node.php in baseFieldDefinitions() is

    $fields['uid'] = FieldDefinition::create('entity_reference')
      ->setLabel(t('User ID'))
      ->setDescription(t('The user ID of the node author.'))
      ->setSettings(array(
        'target_type' => 'user',
        'default_value' => 0,
      ));

That's the "base" field.

Still iffy on this.

starting to summarize the changes.

1. +++ b/core/modules/node/lib/Drupal/node/NodeTranslationController.php
   @@ -56,8 +56,10 @@ public function entityFormEntityBuild($entity_type, EntityInterface $entity, arr
   +      // $form['content_translation']['name'] is the equivalent field
   +      // for translation author uid.
   +      $translation['name'] = $form_state['values']['uid'];
   

   Somewhat ironic :-)

2. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
   @@ -34,11 +34,6 @@ protected function prepareEntity() {
        $this->settings = $type->getModuleSettings('node');
   -    $this->settings += array(
   -      'options' => array('status', 'promote'),
   -      'preview' => DRUPAL_OPTIONAL,
   -      'submitted' => TRUE,
   -    );
   

   I don't get how this is related ?

3. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
   @@ -87,15 +80,6 @@ public function form(array $form, array &$form_state) {
   -    // Basic node information.
   -    // These elements are just values so they are not even sent to the client.
   -    foreach (array('nid', 'vid', 'uid', 'created', 'type') as $key) {
   -      $form[$key] = array(
   -        '#type' => 'value',
   -        '#value' => isset($node->$key) ? $node->$key : NULL,
   -      );
   -    }
   -
   

   Dunno if it's really needed either, but yay for cleanup, we're not in D5 anymore :-)
   (Field UI still has a bunch of those...)

4. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
   @@ -153,6 +138,8 @@ public function form(array $form, array &$form_state) {
   +      '#group' => 'revision_information',
   +      '#access' => $node->isNewRevision() || user_access('administer nodes'),
   

   Hm. So each individual field needs to duplicate the same #access boolean logic than the "group container" it goes into. Not ideal :-/

   - We could say '#access' => $form['revision_information']['#access']

   - We could bake that logic into process_group() and make #access automatically inherit the one set to the group container ? Probably not, magic #access messing sounds dangerous

   - Inherently, the #access really belongs to each and every one of the individual fields, and the group container should just render to nothing if it has no visible children left.

   The latter would IMO be the soundest approach: access is inherently by field, grouping is presentational.

   Not sure how easy it would be - but that might be something field_groups has already had to solve in contrib. #access rules are by field in D7, and I'd think the module tries to not display empty fieldsets if all the fields it contains are hidden ?

5. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
   @@ -423,15 +418,15 @@ public function buildEntity(array $form, array &$form_state) {
        // A user might assign the node author by entering a user name in the node
        // form, which we then need to translate to a user ID.
   -    if (!empty($form_state['values']['name']) && $account = user_load_by_name($form_state['values']['name'])) {
   +    if (!empty($form_state['values']['uid']) && $account = user_load_by_name($form_state['values']['uid'])) {
          $entity->setOwnerId($account->id());
   

   The comment could use an update.

   Also, is this really needed now that names match ?
   validate() has already validated that user_load_by_name($form_state['values']['uid']) exists, and the parent::buildEntity() automatically assigns field values from form values if names match.

   Similarly, processing for 'created' below could be simplified ?
   (isn't there an issue to have smart 'created' fields defaulting to REQUEST_TIME or something ? @Berdir ?)

   Also, what if the patch didn't remove the current
   $node->setOwnerId(\Drupal::currentUser()->id());
   $node->setCreatedTime(REQUEST_TIME);
   in prepareEntity() ? Wouldn't that simplify things here as well ?
   I.e. no need to set defaults at submit time if missing from the submitted form values, fill in defaults directly in the entity before building the form - which is more or less what "Entity Field default values" do.

6. +++ b/core/modules/datetime/datetime.module
   @@ -46,7 +46,8 @@ function datetime_element_info() {
      $types['datetime'] = array(
        '#input' => TRUE,
        '#element_validate' => array('datetime_datetime_validate'),
   -    '#process' => array('datetime_datetime_form_process'),
   +    '#process' => array('datetime_datetime_form_process', 'form_process_group'),
   +    '#pre_render' => array('form_pre_render_group'),
   

   So, this is the part that bugs me most.
   Basically, for this to work, we need to add form_process_group & form_pre_render_group to any form #type that has a potential to be moved into a #group.

   So far, only 'container' types had those: fieldset, details, container
   Now we need to add it to: datetime, textfield, textarea, checkbox
   as well, which sounds a bit arbitrary, and not too scalable.

   Not sure how to adress that, but ideally #group should "just work" on any element type, not just on the ones that provision for it.
   That's probably better left for a separate issue, but I'd think form_process_group() & form_pre_render_group() should just run on every sub-element in a form / render array.
   No biggie for form_pre_render_group(), it's an early opt-out if $element['#group'] is not set. Might be more problematic for form_process_group(), at least as is.

@Gabor #64:

2. Not opposed to cleaning that up, was just wondering about the status of the change (side cleanup vs. consequence of the other changes)

4; Actually, 'uid', 'name', etc.. are Fields, and as such, are subject to the field access API. It can probably qualify as a bug that the node form currently bypasses field-level access rules and assigns hardcoded #access values for 'uid', 'created', etc... (on the parent container in current HEAD, on the parent container + the $element itself in the patch)
That's going to be an issue when converting those $elements to actual widgets, since widgets assign #access based on field_access API.

We don't necessarily have to fix this here for each of the individual $elements touched by the patch, but yeah, we should IMO at least try to avoid having the group container making assumptions about the #access rules of its children, and instead not render itself if it has no visible children.

5. - simplification of the submit flow for 'uid' & 'created':
Right I was going a little ahead of things. Cleaning this is a topic for if/when those fields get converted to using actual entity_ref & date widgets.

6. Opened #2190333: Make #group FAPI / render feature work on all form/render #types out of the box
Meanwhile, I'm wondering whether the patch here would be better off adding 'form_process_group' & 'form_pre_render_group' manually to each individual $element where a '#group' property is added ('uid', 'created', etc...), rather than adding it to a couple arbitrary element types in hook_element_info() ?

Thanks Gabor! This looks good to me.

68: 2177469-move-node-base-widgets-top-68.patch queued for re-testing.

#57 + #58 pretty much re-expresses exactly my concerns.

How will this work for e.g. the author fields in CommentFormController?

1. +++ b/core/modules/node/lib/Drupal/node/NodeFormController.php
   @@ -316,11 +308,11 @@ public function validate(array $form, array &$form_state) {
        // Validate the "authored by" field.
   -    if (!empty($form_state['values']['name']) && !($account = user_load_by_name($form_state['values']['name']))) {
   +    if (!empty($form_state['values']['uid']) && !($account = user_load_by_name($form_state['values']['uid']))) {
   
   @@ -423,15 +415,15 @@ public function buildEntity(array $form, array &$form_state) {
        // A user might assign the node author by entering a user name in the node
        // form, which we then need to translate to a user ID.
   -    if (!empty($form_state['values']['name']) && $account = user_load_by_name($form_state['values']['name'])) {
   +    if (!empty($form_state['values']['uid']) && $account = user_load_by_name($form_state['values']['uid'])) {
          $entity->setOwnerId($account->id());
        }
   

   Are you sure that these changes do not cause anonymous users to be able to set an arbitrary comment author? (impersonation)

   Where and how is the differentiation between trusted author information and untrusted author information made now?

2. +++ b/core/modules/system/system.module
   @@ -469,8 +470,8 @@ function system_element_info() {
   +    '#process' => array('form_process_checkbox', 'ajax_process_form', 'form_process_group'),
   +    '#pre_render' => array('form_pre_render_checkbox', 'form_pre_render_group'),
   

   Did we verify whether the #process and #pre_render callbacks for #group should not actually come first instead of last? I'm concerned whether earlier callbacks might depend on the group adjustments?

#57 + #58 pretty much re-expresses exactly my concerns.

I don't see how "have form $elements be named the same that the underlying base field they edit, rather than some arbitrary name" could be something that adds confusion ?

#57 + #58 is IMO mostly about 'uid' being an extremely confusing name for the "node author" field in D8. It's no longer an id, it's an entity_reference field.
The field should be named 'author' or 'owner', because that's what it actually is.

Code like:
user_load_by_name($form_state['values']['uid'])
array('%name' => $form_state['values']['uid'])
does look weird. But that's because 'uid' is the wrong name for the field to begin with.
But renaming the field is outside the scope of this patch.

Also, the goal is to move "node author" to an actual entity_reference widget - then that custom code disappears, the form element is automatically generated under $form[$field_name] like all other widgets, and this doesn't bug anyoine anymore :-)

1. I'm still (very) concerned about the $form structure data binding, but I do not want to hold up progress.

2. If possible, it would be great to see an explanation for the question I asked in #73 already: How does this work for the author fields in CommentFormController?

   → Happy to have that in a separate issue, since CommentFormController is the next best conversion in light of the changes here. Though I'm fairly confident that we'll run into hard problems.

Oops - Convert theme_details() to Twig landed :-/

Hm, this breaks the admin/modules page, which uses 'details' with just a '#description' & no children - the description is the content.

I guess both use cases are valid:
- display a 'details' with its description even if empty
- setup a 'details' container, possibly with a description, but hide it if all its children turn out to be hidden.

So maybe we should add an explicit #hide_if_empty boolean param ?

Reverted #67 allowing details to be rendered always, I see no reason to hide them.
The same as introduce #hide_if_empty that will confuse much more.
Suppose better to apply the same #access as their children have.

Agree with @sun $41.4

→ The form_process_group() and form_pre_render_group() callbacks should be removed and the concept should be refactored directly into FormBuilder.

this needs separate issue

PS: comment form would be really nightmare to convert, but I think better prepare widgets ant try to apply them to form, there's not so much left to make it

I checked the extend page, manage fields on article, created a node, edited it, and did something with revisions.

Is there any specific places to check this manually?
Is there any other place with a known empty group that we can check?

(haven't read all the comments yet, or the whole patch)

Reverted #67 allowing details to be rendered always, I see no reason to hide them.
The same as introduce #hide_if_empty that will confuse much more.
Suppose better to apply the same #access as their children have.

Sorry, but, as explained in #63.4 and #65.4 , this is inherently wrong.
It's a bug that the $elements for 'author', 'created'... currently hardcode an #access without going through the Entity Field Access API.
Therefore, the "details" container cannot hardcode assumptions on which #access its children have (and each child might have different ones, too).

Now, we can consider that this bug of hardcoded #access exists in HEAD already and that we leave it unresolved here to move forward, but that's something we'll need to solve in a major followup.

We *do* have cases of :
a) 'details' that need to be displayed even if they have no children (the 'details' on admin/modules)
*and*
b) 'details' that need to be hidden if they have no visible children (the 'details' in the node edit form)
So I don't see how we avoid a #flag property to specify which behavior is wanted case by case.

Agree with @sun $41.4

→ The form_process_group() and form_pre_render_group() callbacks should be removed and the concept should be refactored directly into FormBuilder.

this needs separate issue

Yes, that's #2190333: Make #group FAPI / render feature work on all form/render #types out of the box.

Now, we can consider that this bug of hardcoded #access exists in HEAD already and that we leave it unresolved here to move forward, but that's something we'll need to solve in a major followup.

Does it makes sense to postpone the issue on #2190333: Make #group FAPI / render feature work on all form/render #types out of the box

Works for me if green. Thanks Gabor.

so it is as if everything has a default optional false, that means always show the group, whether or not there are children?

and we are only specifying optional true where we want details hidden when there are no children?

I looked at the interdiff, but I dont see where optional is documented as being a possible array value.

1.
I'm not sure. This is "fapi documentation"?

Maybe something like

diff --git a/core/includes/form.inc b/core/includes/form.inc
index 74c66fd..e31566e 100644
--- a/core/includes/form.inc
+++ b/core/includes/form.inc
@@ -990,7 +990,10 @@ function theme_fieldset($variables) {
  *   An associative array containing:
  *   - element: An associative array containing the properties of the element.
  *     Properties used: #attributes, #children, #collapsed, #collapsible,
- *     #description, #id, #title, #value.
+ *     #description, #id, #optional, #title, #value.
+ *
+ * Note: Leave #optional not set if the details should always be shown. Set
+ * #optional to TRUE if the details should be hidden when empty.
  *
  * @ingroup themeable
  */

and an update to the doc git repo that (should) document details. #1892968: Fieldsets vs. Details: When to use what and why?
Maybe the "Note:" part the kind of information that should not be in form.inc but should be in https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen...

---
2.
a. I tried manually testing the #optional by commenting out (and uncommenting) the optional = true lines for authoring info and promotion, and trying to create an article as anonymous (giving anon permission to create article). And I can see it makes sense to have authoring and promotion optional as that user cannot see what are in those details anyway. Good! (I could not figure out what situation might make the revision info detail empty/childless.)

b. I was going to try putting optional on another form (like the admin/modules) just to see if it would hide those details, but I got as far as ModulesListForm and couldn't figure out where to put it.

Sorry, but why are we proceeding into the full theme layer and uglifying the theme template, if nothing is to be rendered in the first place?

The case of #optional with no element children should be caught by the #pre_render function already. Simply setting #printed to TRUE should cut it.

Thanks, that looks better.

Cool. Back to RTBC then ?

This all looks like really great clean-up to me, apart from this gem:

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
@@ -155,17 +155,17 @@ protected function doTestAuthoringInfo() {
-        'name' => $user->getUsername(),
...
+        'uid' => $user->getUsername(),

Um. Wat? :)

When I asked about this in IRC, Gábor explained that basically this patch makes it so that $form['X'] <====> $node->x and the property that's being set is $node->uid, not $node->name. And we don't typically name fields after their widgets; e.g. a field_timestamp never meant it would have an integer number widget, so this is the same. I guess this is fair enough, but it looks wack-tastical. I'd recommend a follow-up issue, except I'm not really sure what we would do to change it. :\

Anyway, since that's just status quo, not going to hold this patch up on that. Especially since it unblocks at least 3-4 other things.

Committed and pushed to 8.x. Thanks!