Not sure where this belongs exactly.

I updated to 2.x-dev and tested my profile, which has a date field. And it stopped working and I got a validation error about an invalid date format when it was clearly valid.

Some fun debugging later, I tracked this down to the fact that ProfileSelect is a form element which means it has #input TRUE (not actually explicitly set, but all form elements get that automatically). That means it owns everything within it and the value is set in the form_state values.

When nested elements are then processed in \Drupal\Core\Form\FormBuilder::handleInputElement(), it sees that there already are values and doesn't update it:

    // Set the element's value in $form_state->getValues(), but only, if its key
    // does not exist yet (a #value_callback may have already populated it).
    if (!NestedArray::keyExists($form_state->getValues(), $element['#parents'])) {
      $form_state->setValueForElement($element, $element['#value']);
    }

And then the values for the date field are incomplete and everything falls apart :)

Not exactly sure what needs to happen. I see that form elements like Table are also form elements and it works fine for them obviously. They do have a value callback, so maybe what's needed is a custom value callback that only returns its own value?

Comments

Berdir created an issue. See original summary.

berdir’s picture

Turns out Table is actually equally broken:

To reproduce, just throw this in NodeForm or any other form:


   $form['foo'] = [
      '#type' => 'table',
      '#header' => ['WTF'],
    ];
    $form['foo'][0][0] = [
      '#type' => 'datetime',
      '#default_value' => NULL,
      '#date_increment' => 1,
      '#date_timezone' => drupal_get_user_timezone(),
    ];

I'll open a core issue as well for this.

berdir’s picture

Forgot about this again but then the customer found this as well, created a PR now that converts the base class to a trait, that *seems* to be working, lets see what the tests have to say: https://github.com/drupalcommerce/commerce/pull/721

We could also keep the old class as BC, but I guess you don't care about that yet.

berdir’s picture

Status: Active » Needs review
mglaman’s picture

We could also keep the old class as BC, but I guess you don't care about that yet.

I think we'll be fine. I have a feeling most people are extending classes which already extend that class.

bojanz’s picture

Status: Needs review » Fixed

Merged the PR in 0e6f8627cf44d94f132f1d8c4d6e7e24654eb324, thanks!

replicaobscura’s picture

Adding related issue about PaymentGatewayForm

replicaobscura’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.