Contrib.social

HTML entities in Tour tip labels get double-escaped

Open on Drupal.org →
Created on 24 December 2021, about 3 years ago
Updated 16 February 2023, almost 2 years ago

Problem/Motivation

If a Tour tip label includes HTML tags or characters that will be converted to HTML entities by Html::escape() (e.g. ' to &#039;), those will appear double-escaped in the resulting Tour tip. For example, the label "What''s New with text in <strong>bold</strong>" would get output as this:

What&#039;s New with text in &lt;strong&gt;bold&lt;/strong&gt;

Steps to reproduce

1. Create a Tour with a tip with a label with text that will be escaped by Html::escape(). This YAML would work ( tour.tour_.test-admin.yml, attached):

langcode: en
status: true
dependencies:
  module:
    - system
id: test-admin
label: 'Admin page'
module: system
routes:
  -
    route_name: system.admin
tips:
  test-admin-main:
    id: test-admin-main
    plugin: text
    label: 'What''s New with text in <strong>bold</strong>'
    weight: 1
    attributes:
      class: no-nub
    body: 'This is the admin page of the Drupal site.'

2. Start Tour.
3. See tip with double-escaped text.

Proposed resolution

This is a caused by code in core/modules/tour/src/TourViewBuilder.php that calls Html::escape():

$output = [
  'body' => $body,
  'title' => Html::escape($tip->getLabel()),
];

This text does not need to be escaped here because it's already being done somewhere else. If we remove the call to Html::escape(), the resulting label will have the text properly escaped (once):

What's New with text in <strong>bold</strong>

Remaining tasks

1. Patch code to remove call to Html::escape().
2. Write test (is that needed?).
3. Commit change.

User interface changes

before

after

🐛 Bug report
Status

Fixed

Version

9.5

Component
Tour  →

Last updated 6 days ago

Created by

🇺🇸United States jrb Raleigh-Durham Area, NC, USA

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

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment almost 2 years ago →
    🇺🇸United States smustgrave

    Retested #36 following the issue summary.

    Posted before/after screenshots to issue summary.

    #35 looks to have been addressed.

  • Comment almost 2 years ago →
    System Message
    • lauriii → committed 18c67bff on 10.1.x 
      Issue #3255895 by jrb, murilohp, harshitthakore, xjm: HTML entities in...
  • Comment almost 2 years ago →
    🇫🇮Finland lauriii Finland

    Confirmed that the title is not being used anywhere else than what was pointed out in #36.

    Committed 18c67bf and pushed to 10.1.x. Thanks!

    Leaving open for 10.0.x and 9.5.x backport.

  • Comment almost 2 years ago →
    🇧🇷Brazil murilohp

    Hey @lauriii, here's a patch for 9.5, #36 is applying corrcetly on 10.0.

  • Status changed to Needs work almost 2 years ago
  • Comment almost 2 years ago →
    needs-review-queue-bot

    The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇮🇳India mrinalini9 New Delhi

    Hi,

    It seems that the changes in patch #42 are already present in 10.1.x branch. So, reroll is not needed for 10.1.x.

    Thanks & Regards,
    Mrinalini

  • Status changed to RTBC almost 2 years ago
  • Comment almost 2 years ago →
    🇫🇮Finland lauriii Finland

    #42 is not supposed to work with 10.1.x since it's only for 9.5.x. Moving back to RTBC.

  • Comment almost 2 years ago →
    System Message
    • lauriii → committed bfecafcf on 10.0.x 
      Issue #3255895 by jrb, murilohp, harshitthakore, xjm: HTML entities in...
  • Comment almost 2 years ago →
    System Message
    • lauriii → committed ae16bcd7 on 9.5.x 
      Issue #3255895 by jrb, murilohp, harshitthakore, lauriii, xjm: HTML...
  • Status changed to Fixed almost 2 years ago
  • Comment almost 2 years ago →
    🇫🇮Finland lauriii Finland

    Cherry-picked to 10.0.x and committed #42 to 9.5.x. Thank you!

  • Comment almost 2 years ago →
    System Message

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

  • Status changed to Fixed over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States Kristen Pol Santa Cruz, CA, USA

    Was reviewing this issue. I'm not understanding why smustgrave and ameymudras and possibly others were not given issue credit as they had genuine contributions IMO but this issue is closed so maybe it won't be changed.

  • Comment over 1 year ago →
    🇫🇮Finland lauriii Finland

    I didn't credit @smustgrave, @ameymudras or anyone else for posting screenshots to this issue since this issue did not benefit from screenshots besides the one that was part of the original issue summary. What it needed was code review. What comes to crediting code review, we only give credit for substantial code reviews. In the case of this issue, it could have been explaining why the change that is being proposed in \Drupal\tour\TourViewBuilder is fine.

  • Comment over 1 year ago →
    🇺🇸United States Kristen Pol Santa Cruz, CA, USA

    Thank you for the explanation. I’ll need to figure out how to better find issues to work on and then help my coworkers do the same. I need to better understand what actually requires testing given these issues I’ve been reviewing seem to not need it.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024