Problem/Motivation

Currently in ConfigSingleExportForm to select and replace the field including its wrapper, we are using the CSS "has" selector: select('*:has(>select[name="config_name"])'). This works but is complex.

Proposed resolution

Add a data-drupal-wrapper-selector attribute to the wrapper element so that it can be directly targeted easily.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3603310

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

fathershawn created an issue. See original summary.

fathershawn opened merge request !16093

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Issue summary: View changes
fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Active » Needs review

ConfigSingleExportForm working properly using the new selector to replace the config name select element and to return the export text area with the result.

smustgrave’s picture

smustgrave commented

Question could this be a breaking change for someone's custom build? Know it's a task does it need coverage or fact tests pass the coverage?

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Since this adds a new data attribute I don't think it would break anything.

The updated usage in ConfigSingleExportForm implicitly tests it. I'm happy to build a direct test if that's desired.

smustgrave’s picture

smustgrave commented

Mind rerunning the failed test job?

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Ran locally and ConfigDraggableListBuilderTest passes so not going to start a new pipeline.

godotislate made their first commit to this issue’s fork.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

Rebased and ConfigDraggableListBuilderTest has failed twice.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Thank you @godotislate! Fixed and pushed.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

Okay so ConfigDraggableListBuilderTest is fixed and now passing but KernelTestHttpRequestTest although it passes locally. I don't see anything wrong there. Anyone see it?

nitinkumar_7’s picture

nitinkumar_7 commented

I don't immediately see anything in this change that would affect KernelTestHttpRequestTest. Since the failure is not reproducible locally, it may be worth checking the CI logs to see exactly which assertion is failing and whether its an intermittent issue.

nitinkumar_7’s picture

nitinkumar_7 commented

If the failure is unrelated to the changes in this MR, a rerun should help confirm that; otherwise the failing assertion should point us toward any unexpected side effects.

godotislate’s picture

godotislate
he/him
commented

Think it's unrelated. The test generates a random string to use as markup, but the generated random string might be sanitized when rendered and not be identical to the original string.

I'll open a new issue.

godotislate’s picture

godotislate
he/him
commented

Opened #3613115: [random test failure] KernelTestHttpRequestTest::testKernelTestRoute fails intermittently because random string is not sanitized for #15.

godotislate’s picture

godotislate
he/him
commented

For now, I re-ran the kernel test and it passed.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback appears to be addressed to me.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs review

Q on the MR.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

@godotislate There was a logic error in the test, now fixed. Before it checked for the export empty on type change immediately after checking for export empty on change to an empty name. Added a request to re-fill the export in between and the test failed with a wrong selector in the form and passes with our code change.

fathershawn’s picture

fathershawn
Primary language English
Location New York
commented

I have a LayoutBuilder test and Toolbar test failing. Seems unrelated.

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

Re-ran tests, looks good.

  • godotislate committed 084bd436 on 11.x 
    task: #3603310 Use a data attribute to streamline form element wrapper...

  • godotislate committed 23942ea0 on main 
    task: #3603310 Use a data attribute to streamline form element wrapper...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 23942ea to main and 084bd43 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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