Problem/Motivation

ConfigTranslationCacheTest fails if 

$field_label_fr = $this->randomString();

contains a ' character. This is because the test is using assertEscaped(). Field labels are no longer escaped in the admin interface, they are XSS::adminFilter()'ed. This was changed in #2409559: User should be able to identify required fields from field listing (unintentionally).

Steps to reproduce

Change the line to $field_label_fr = $this->randomString() . "'some quotes'"; - the test will always fails.

Proposed resolution

Use #plain_text instead of #value to escape these values like they used to be.

MR for fix: https://git.drupalcode.org/project/drupal/-/merge_requests/13911
MR for proof: https://git.drupalcode.org/project/drupal/-/merge_requests/13913
MR for fail: https://git.drupalcode.org/project/drupal/-/merge_requests/13914

Note this is different from $translatable_field_setting = $this->randomString(); - which uses escaping. That's because this appears on the translation form. The label is part of the admin listing page which uses XSS filtering.

Remaining tasks

User interface changes

None

Introduced terminology

N/a

API changes

None

Data model changes

None

Release notes snippet

N/a

Issue fork drupal-3559326

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

alexpott created an issue. See original summary.

alexpott opened merge request !13911

alexpott opened merge request !13913

alexpott opened merge request !13914

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
Status: Active » Needs review

Created 3 MRs. 1 for the real fix, 1 for a proof and 1 to show the fail.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#2409559: User should be able to identify required fields from field listing

This was introduced by #2409559: User should be able to identify required fields from field listing which changed the label escaping strategy from escaping to admin filtering...

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

Thanks for this incredibly thorough bug report and fix! Let's smash it.

  1. The issue summary is totally clear and complete.
  2. The fail MR fails thusly by always having the ' in the label, and not fixing the assertions:
        Config Translation Cache (Drupal\Tests\config_translation\Functional\ConfigTranslationCache)
     ✘ Field config translation
       ┐
       ├ Behat\Mink\Exception\ExpectationException: The string "TM4s>&cAsome'quote" was not found anywhere in the HTML response of the current page.
...
       │ /builds/issue/drupal-3559326/core/modules/config_translation/tests/src/Functional/ConfigTranslationCacheTest.php:168
  3. The proof MR passes, by having the fix, and forcing the ' into the label.
  4. The fix MR is the clean fix, and the pipeline is all green (tests pass, no linting problems, etc).

I see nothing else to improve here. In spite of my usual preference for verbosity and comments, I don't think we want a comment to explain why we're not using assertEscaped(). We don't comment about all the wrong ways to do things we're not using. 😂 There's nothing confusing or intuitive about pageTextContains().

Ship it!

Thanks again,
-Derek

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
dww’s picture

dww
we/he/they
commented
Title: Random fail in ConfigTranslationCacheTest due using assertEscaped on XSS filtered text » Random fail in ConfigTranslationCacheTest since labels changed from escaped to XSS filtered text
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

My RTBC in #7 was just before the "Change approach" commit and summary update. The new summary explains why this started happening recently. That's even better.

I did some slight edits for formatting, and to clarify that the test is not using assertEscaped() "incorrectly". Also trying to get the title more accurate to the new approach.

I'll wait to RTBC until the dust settles and the MR pipelines are as expected.

  • longwave committed cc4e5423 on 11.3.x 
    fix: #3559326 Random fail in ConfigTranslationCacheTest due using...

  • longwave committed 6c71422c on 11.x 
    fix: #3559326 Random fail in ConfigTranslationCacheTest due using...

catch closed merge request !13911

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 11.3.x-dev
Status: Needs review » Fixed

Oops, crossposted while I was committing this! It looks good to me so let's get it in.

Committed and pushed 6c71422c72b to 11.x and cc4e5423465 to 11.3.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.

catch reopened merge request !13911

Status: Fixed » Closed (fixed)

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