Problem/Motivation

Discovered during #1825952: Turn on twig autoescape by default. The class docblock for \Drupal\Core\Template\Attribute has a few typos and odd sentences, and does not mention a critical piece of information (that the output is automatically sanitized).

Proposed resolution

Clean it up. Patch attached.

CommentFileSizeAuthor
#18 interdiff-2290143-16-18.txt843 bytesamitgoyal
#18 drupal-2290143-18.patch2.13 KBamitgoyal
#16 drupal-2290143-16.patch2.19 KBer.pushpinderrana
#13 interdiff-11-13.txt623 bytescs_shadow
#13 drupal-2290143-13.patch2.18 KBcs_shadow
#11 interdiff-3-11.txt1.68 KBcs_shadow
#11 drupal-2290143-11.patch2.17 KBcs_shadow
#3 interdiff-0-3.txt877 bytescs_shadow
#3 drupal-2290143-3.patch1.77 KBcs_shadow
attribute.patch1.61 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

The docs here look good. Wow, attributes are kind of convoluted, but after reading through the code for the various attribute-related classes, I agree with the assessment that the output is sanitized as described.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -22,7 +22,8 @@
- * individual parts of the attribute may be printed first.
+ * Individual attributes may be printed first, and will then automatically be
+ * excluded when the remaining attributes are printed:

The "automatically be excluded" part is no longer true: https://www.drupal.org/node/2240005

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
FileSize
drupal-2290143-3.patch1.77 KB
interdiff-0-3.txt877 bytes

Modified documentation as suggested in #2. The example below also needs to be modified, though I'm not sure how to use "without" filter in php code since the example uses the filter in twig.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

I'm going to let Cottser (please!) review this one. :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented 
+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -22,8 +22,11 @@
+ * @link https://www.drupal.org/node/2212845 without filter @endlink
+ * to prevent attributes that have already been printed from being printed

Use the "without filter" what? Noun needed :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Oh, it's the "without" (some kind of) filter. The "without" Twig filter? Punctuation matters. :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Hm, also, I don't think we have many (or any) references to change records in the codebase, since they're D7-to-D8 specific and irrelevant after. It'd be better to add a code snippet, or update it (as suggested).

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Needs work
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

Oh, just a note -- it's perfectly okay to put template markup in a docblock @code section. I'd add something like "in your Twig template" above the code snippet for clarity.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Novice
cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
FileSize
drupal-2290143-11.patch2.17 KB
interdiff-3-11.txt1.68 KB

I changed to the original snippet to point out that indeed all the attributes will be printed ALWAYS. Added a new snippet that shows how to use the "without" filter.
And also, added the punctuation mentioned in #6. Thanks for pointing it out.

Waiting for some feedback.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Status: Needs review » Needs work

This is looking much better, thanks everyone!

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -22,14 +22,27 @@
+ * @code
+ *  <cat class="{{ attributes.class }} my-custom-class"{{ attributes|without('class') }}>
+ *  {# Produces <cat class="cat black-cat white-cat black-white-cat" id="socks" #}
+ * @endcode

Missing closing > and missing 'my-custom-class', and no need to indent within the @code..@endcode according to https://www.drupal.org/node/1354#code.

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
FileSize
drupal-2290143-13.patch2.18 KB
interdiff-11-13.txt623 bytes

Added missing > and 'my-custom-class'.
@Cottser, left the indent within @code...@endcode as it is to maintain consistency with the other places in the same file.

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Needs work » Needs review
LinL’s picture

LinL CreditAttribution: LinL commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies, tagging for reroll.

er.pushpinderrana’s picture

er.pushpinderrana CreditAttribution: er.pushpinderrana commented
Status: Needs work » Needs review
FileSize
drupal-2290143-16.patch2.19 KB

Rerolled Patch.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

As xjm pointed out above, the link to https://www.drupal.org/node/2212845 in the text about the "without" filter is going to a change record. This is undesirable -- it should go to a d.o documentation page if possible, and if not... well maybe we don't need the link? The example I think is pretty clear...

amitgoyal’s picture

amitgoyal CreditAttribution: amitgoyal commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
drupal-2290143-18.patch2.13 KB
interdiff-2290143-16-18.txt843 bytes

Please review revised patch attached as I have removed "without" filter link as per #17.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks!

alexpott’s picture

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

Committed bd0f39f and pushed to 8.0.x. Thanks!

  • alexpott committed bd0f39f on 8.0.x 
    Issue #2290143 by cs_shadow, amitgoyal, er.pushpinderrana, xjm: Improve...

Status: Fixed » Closed (fixed)

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