Problem/Motivation

#3382510: Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms landed, yay! Unfortunately it introduced a regression 🐛

So I tried adopting it in my contrib module (see #3394172-4: Adopt Drupal core 10.3 config validation infrastructure), and found a bug: the call to FormStateInterface::setErrorByName() is wrong.

Root cause: despite the name of that method, it does not accept a form element name (as in #name), but a variation thereof: implode('][', $element['#parents']) (this is clear if you look at \Drupal\Core\Form\FormState::setError()).

Steps to reproduce

Trigger any validation error, and you'll notice that the form validation error is not associated with any form element, only with the entire form:

Proposed resolution

One-line change:

- $form_element_name = $map["$config_name:$property_path"]->elementName;
+ $form_element_name = implode('][', $map["$config_name:$property_path"]->elementParents);

yields:

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

CommentFileSizeAuthor
Screenshot 2023-11-03 at 11.09.32 AM.png171.95 KBwim leers
Screenshot 2023-11-03 at 11.10.08 AM.png190.32 KBwim leers

Issue fork drupal-3398974

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Status: Active » Needs review
Related issues: +#3364506: Add optional validation constraint support to ConfigFormBase

Arguably this needs tests. OTOH, this is such a trivial change/fix that that seems … excessive.

Why wasn't this caught? Doesn't this have test coverage? Well, we were relying on the test coverage that #3364506: Add optional validation constraint support to ConfigFormBase had introduced: UpdateSettingsFormTest. That explicitly tests this:

    $this->assertTrue($this->assertSession()->fieldExists('update_notify_emails')->hasClass('error'));

But … that just happens to be a case where there is no nesting (no #tree ⇒ TRUE).

Wim Leers opened merge request !5239

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

seems the suggestion in the MR is to remove the property. Not sure if that deserves it's own follow up or if it's simple enough here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Closed (duplicate)

I've merged this into #3398982: ConfigFormBase + validation constraints: support non-1:1 form element-to-config property mapping again as they are extremely related and really need fixing together.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

👍🙏

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Closed (duplicate) » Needs review

Well it turns out the other part of #3398982: ConfigFormBase + validation constraints: support non-1:1 form element-to-config property mapping again is super complex. Let's get this done first.

alexpott opened merge request !5259

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

I don't fully understand the entire scope of the slack discussion, but this patch is not very complicated and it looks great. I can't run the test only changes on the MR, but the second MR fails where it is expected to fail. +1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Opened #3399510: Replace calling FormState::setErrorByName() with an empty string with a new method at catch's request

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Reviewed & tested by the community » Needs work

It took me a few minutes to understand the

if ($property_path === '') {
          // There is a map to a non-existing config key. Try to work backwards.
          $property_path = $violation->getParameters()['@key'] ?? '';
        }

dark magic 🧙

I think we can make it less mysterious 😄

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

@Wim Leers nice idea - I agree and applied your suggestion.

  • catch committed 97e6267a on 11.x 
    Issue #3398974 by alexpott, Wim Leers, borisson_: Follow-up for #3382510...

  • catch committed b980612a on 10.2.x 
    Issue #3398974 by alexpott, Wim Leers, borisson_: Follow-up for #3382510...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Pushed a better solution than what I suggested on the MR 🤓

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.2.x-dev
Status: Needs review » Fixed

I just committed/pushed this already. Please open a follow-up or maybe we can quick fix it here.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » wim leers
Status: Fixed » Needs work

Okay, on it!

Wim Leers opened merge request !5260

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » alexpott
Status: Needs work » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

This change makes sense to me, and I think it makes the error messages a little clearer. That's not a bad thing. No objections.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed f9109dc8600 to 11.x and 56aeb82270c to 10.2.x. Thanks!

Thanks @Wim Leers

  • alexpott committed f9109dc8 on 11.x 
    Issue #3398974 follow-up by Wim Leers: Follow-up for #3382510:...

  • alexpott committed 56aeb822 on 10.2.x 
    Issue #3398974 follow-up by Wim Leers: Follow-up for #3382510:...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: alexpott » Unassigned

Thanks!

See y'all in #3398982: ConfigFormBase + validation constraints: support non-1:1 form element-to-config property mapping again 😊

Status: Fixed » Closed (fixed)

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