Problem/Motivation

Twig allows to create variables inside the templates using the set command. When one of these variables is used as an Attribute, the rendering of this attribute throws the following error:
Error: Call to undefined method Twig\Markup::render() in Drupal\Core\Template\Attribute->__toString() (line 329 of core/lib/Drupal/Core/Template/Attribute.php)

Steps to reproduce

Add this code into one of your twig templates (eg. page.html.twig)

  {% set test_attr %}
    This is a test
  {% endset %}
  {%
    set attributes = create_attribute()
     .setAttribute('data-test', test_attr)
  %}
  {{ attributes }}

Expected result would be to display data-test="This is a test".
Current result is a fatal error.

Proposed resolution

In \Drupal\Core\Template\Attribute::createAttributeValue(), expand the condition on instances of MarkupInterface to be true on instances of \Stringable objects.

CommentFileSizeAuthor
#9 3537801-nr-bot__ckpsfqf.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3537801

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

duaelfr created an issue. See original summary.

duaelfr opened merge request !12831

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented

What happens though if you add markup like 

 {% set test_attr %}
    <p>This is a test<p>
  {% endset %}
duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented

The Attribute object already filters HTML from values so that shouldn't be an issue. I believe this is covered by some tests.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Gotcha, then seems like a fine change. The test only run is also showing https://git.drupalcode.org/issue/drupal-3537801/-/jobs/5996750


1) Drupal\Tests\Core\Template\AttributeTest::testAttributeValues with data set "twig-markup" ([Twig\Markup Object (...)], ' title="foo"')
Error: Call to undefined method Twig\Markup::render()
/builds/issue/drupal-3537801/core/lib/Drupal/Core/Template/Attribute.php:329
/builds/issue/drupal-3537801/core/tests/Drupal/Tests/Core/Template/AttributeTest.php:376
ERRORS!
rajesh.vishwakarma’s picture

rajesh.vishwakarma commented

It working fine after adding the patch. I tested with Drupal 11.2.2
RTBC +1

pdureau’s picture

pdureau commented
Assigned: Unassigned » pdureau

I am not comfortable with the starting point, because this is not supposed to happen:

  {% set test_attr %}This is a test{% endset %}
  {%  set attributes = create_attribute()  .setAttribute('data-test', test_attr) %}

For a simple string like that, this Twig syntax is available:

  {% set test_attr = "This is a test" %}
  {%  set attributes = create_attribute()  .setAttribute('data-test', test_attr) %}

But I am OK with the change, replacing ($value instanceof MarkupInterface) by ($value instanceof Stringable), because:

  • MarkupInterface extends Stringable.
  • Duaelfr told us that the Attribute object already filters HTML from values

I will wait a bit, to not rush, and I will merge if no objections.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3537801-nr-bot__ckpsfqf.txt91 bytes

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

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

smustgrave’s picture

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

Was a clean rebase.

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

  • catch committed dfbc64a2 on 11.x 
    Issue #3537801 by duaelfr, smustgrave, pdureau: Fatal error when using a...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

#8 is a good explanation for this being OK.

I updated an inline comment via gitlab suggestions just above the change here - it was still talking about 'safe string' objects which was an early Drupal 8 concept that eventually became MarkupInterface. We're slowly moving towards Stringable elsewhere and this is consistent with that, the conversion from markup to plain text happens immediately after that line, so any HTML in there will be escaped - we're not actually using MarkupInterface to preserve markup in this specific case, it's just saving people casting them to string (and escaping) themselves.

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

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

Maintainers, please credit people who helped resolve this issue.

catch closed merge request !12831

pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned

Status: Fixed » Closed (fixed)

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