Contrib.social

States are processed and added to the wrong attribute since #1427838

Open on Drupal.org →
Created on 28 November 2024, 5 months ago

Problem/Motivation

Since 🐛 password and password_confirm children do not pick up #states or #attributes Active was committed, states are now incorrectly applied to (at least) 'item' form elements.

Specifically the intention of that issue was to change this code:

    // Elements of '#type' => 'item' are not actual form input elements, but we
    // still want to be able to show/hide them. Since there's no actual HTML
    // input element available, setting #attributes does not make sense, but a
    // wrapper is available, so setting #wrapper_attributes makes it work.
    $key = ($elements['#type'] == 'item') ? '#wrapper_attributes' : '#attributes';
    $elements[$key]['data-drupal-states'] = Json::encode($elements['#states']);

To something that includes the password confirm element, and indeed early on in the work for that issue we had this code:

    // Elements of '#type' => 'item' are not actual form input elements, but we
    // still want to be able to show/hide them. Since there's no actual HTML
    // input element available, setting #attributes does not make sense, but a
    // wrapper is available, so setting #wrapper_attributes makes it work.
    $key = ($elements['#type'] == 'item' || $elements['#type'] == 'password_confirm') ? '#wrapper_attributes' : '#attributes';
    $elements[$key]['data-drupal-states'] = Json::encode($elements['#states']);

However, after some discussion it was decided to not target the elements specifically 🐛 password and password_confirm children do not pick up #states or #attributes Active , but try to identify the types of elements that would likely use wrapper attributes instead of attributes.

This resulted in the following code being committed:

    // Elements that are actual form input elements, use '#attributes'.
    // In cases like 'item' that are not actual form input elements or
    // those like 'password_confirm' that have child elements,
    // use #wrapper_attributes.
    $key = (($elements['#markup'] ?? FALSE) === '' && ($elements['#input'] ?? FALSE) === TRUE) ? '#wrapper_attributes' : '#attributes';
    $elements[$key]['data-drupal-states'] = Json::encode($elements['#states']);

This code broke a unit test, which was then fixed in a follow up commit where the fix was to make the testing form 'item' an input element, with no #markup, this is fine for the unit test, but there are plenty of places in core alone that set '#markup' to be a non-empty string, and it's still valid to expect states to apply on those elements, as they did before.

Steps to reproduce

Have a form with some #states set on a regular, plain '#type' => 'item' form element, with #markup a non-empty value, observe that the attribute isn't correctly added to the wrapper on the page. And thus the states doesn't work.

Proposed resolution

I'm not sure if the suggested conditional was better/correct!

In the first instance, we should fix the tests to include non-empty #markup, and then we can try to improve the code trying to detect the case here.

Remaining tasks

  1. Open a MR to fix the tests.
  2. Fix the conditional, to get the tests passing.
🐛 Bug report
Status

Active

Version

11.1 🔥

Component

forms system

Created by

🇬🇧United Kingdom steven jones

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @steven jones
  • Merge request !10378Issue #3490435: States not being processed correctly on item form items. → (Open) created by steven jones
  • Pipeline finished with Failed
    5 months ago
    Total: 559s
    #353141
  • Comment 5 months ago →
    🇬🇧United Kingdom steven jones

    Adding more words to the title of the issue.

    Okay, so the tests failed as expected.
    I wonder what the correct solution is here?

    I'm going to push the change to go back to the sub-optimal check for the type of element fix, as that would actually work correctly, though not optimally.

    Then maybe we need some further discussion about how to resolve this. Do we need a new property on form elements: '#states_attributes_target_key' that gives the name of the attribute to push the states attributes on to?
    Core can set this on item and password_confirm elements to change the target, and anything that wants to do this in contrib can do this too.

  • Pipeline finished with Failed
    5 months ago
    Total: 652s
    #353183
  • Comment 5 months ago →
    cilefen
  • Comment 5 months ago →
    🇬🇧United Kingdom steven jones

    Unassigning myself and setting to needs work, since we need to decide what to do here, but we do have an improved test and sub-optimal fix in the MR

  • Comment 4 months ago →
    dabley

    I have hit the same problem, although in my case the #type is 'container'. My code worked correctly when I applied patch #2700667-104: Notice: Undefined index: #type in Drupal\Core\Form\FormHelper::processStates() → , but as of D10.3.8, that patch no longer applies, and my code no longer works (the #states I've specified are ignored). When I apply the update in MR !10378, then my code works correctly again (fields are hidden or visible as they are supposed to be). So, MR !10378 looks good to me, although I haven't checked to see if it causes anything else to break.

  • Comment 4 months ago →
    vnech

    The patch works for Drupal core v10.3.10

  • Issue was unassigned.
  • Status changed to Needs work about 2 months ago
  • Comment about 2 months ago →
    🇺🇸United States bkosborne New Jersey, USA

    This bit me as well. I agree the logic introduced in the original code that introduced the regression is not correct.

  • Comment about 2 months ago →
    🇺🇸United States bkosborne New Jersey, USA

    This patch works for me to resolve the issue. I know it's not an ideal fix, but this fixes the regression and I think it should be committed to address that so people don't keep hunting around for what caused it.

    I don't understand why someone would create a form type 'item' and set #markup to an empty string and set #input to TRUE. In what scenario does that make sense? I think most people using 'item' elements are in fact adding markup.

  • Comment about 1 month ago →
    🇬🇧United Kingdom catch

    Would be nice to get a form API maintainer review on this issue (shouldn't stop another framework manager committing it if they want to, but I'm not sure I want to without a second opinion on whether there's another option here).

  • Comment 10 days ago →
    🇳🇿New Zealand quietone

    Simplify title. No need to refer to the other issue in the commit message. The [regression] informs the reader there is an older issue involved.

  • Comment 10 days ago →
    godotislate

    Per #6, was there also a regression for '#type' => 'container' that is fixed here? In which case, should there be a test for that as well?

  • Comment 10 days ago →
    🇺🇸United States tim.plunkett Philadelphia

    This should include test coverage for `container`

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024