Problem/Motivation

Currently many templates in core can print out duplicate attributes which could cause annoying bugs for themers and developers. We have all the tools now to prevent this from happening!

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because currently it's easy to have duplicate HTML attributes printed which is not valid.
Issue priority Normal, nothing will blow up if we don't fix it.
Unfrozen changes Unfrozen because it only changes "markup" (really the biggest markup change is the order of attributes in some cases)
Prioritized changes Not a prioritized change.
Disruption No disruption at all, just cleanup.

Proposed resolution

Make things consistent. In the most recent patch all core templates that print attributes have been looked at and the following approach has been taken in dealing with any offenders:

  • When there is one hardcoded attribute added and it's not href, use addClass if it's a class, otherwise use setAttribute.
  • If there are multiple hardcoded attributes added or a hardcoded href, use |without to prevent duplicate attributes from being printed.

href has been special cased because it's the entire reason for the <a> to exist but we could also use setAttribute for it as well.

Remaining tasks

Patch review

Manual testing

To pass manual testing the markup should be the same with and without the patch applied (attribute order can change). Make sure to have render caching turned off or run `drush cr` after applying/reverting the patch.

Core and Classy:

  • block--system-menu-block.html.twig
  • feed-icon.html.twig
  • pager.html.twig
  • status-messages.html.twig
  • vertical-tabs.html.twig
  • views-mini-pager.html.twig
  • views-view-summary-unformatted.html.twig
  • views-view-summary.html.twig
  • views-view-table.html.twig

Classy only:

  • search-result.html.twig
  • taxonomy-term.html.twig

User interface changes

n/a

API changes

n/a

Original report:

Follow-up from #2108771: Remove special cased title_attributes and content_attributes for Attribute creation.

Some templates snuck in with things like <tag class="test"{{ attributes }}> but if you use extra attributes, you must pull them out of attributes.

A big help for this is if the new |without filter gets in and works for attributes as well @see #2114563: Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions.

CommentFileSizeAuthor
#13 Source_of__http___d8_dev_.png25.54 KBjoelpittet
#13 Source_of__http___d8_dev_.png23.59 KBjoelpittet
#11 interdiff.txt17.33 KBstar-szr
#11 2187113-11.patch19.31 KBstar-szr
#10 2187113-10.patch1.98 KBstar-szr
#1 2187113-twig-attribute-cleanup-1.patch5.8 KBjoelpittet

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Active » Needs review
StatusFileSize
new 2187113-twig-attribute-cleanup-1.patch5.8 KB

Very curious if this will pass, also wonder about if |default get's messed up by TwigReference too.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Assigned: joelpittet » Unassigned

Unassigning, please review the approach taken.

star-szr’s picture

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

Thanks for this @joelpittet. Great cleanup!

  1. +++ b/core/modules/field/templates/field.html.twig
@@ -38,11 +38,11 @@ HTML comment.
-  <div class="field-items"{{ content_attributes }}>
+  <div class="field-items {{ content_attributes.class }}"{{ content_attributes }}>

    Should there be a space between the hardcoded class and the attributes.class in cases like these? I thought the Attribute class would always insert spaces as needed. 

  2. +++ b/core/modules/system/templates/maintenance-page.html.twig
@@ -20,7 +20,7 @@
-<body class="{{ attributes.class }}">
+<body class="{{ attributes.class }}"{{ attributes }}>

    Let's just <body{{ attributes }}> in this case. 

  3. +++ b/core/themes/bartik/templates/node.html.twig
@@ -71,7 +71,7 @@
-<article id="node-{{ node.id }}" class="{{ attributes.class}} clearfix"{{ attributes }} role="article">
+<article id="{{ attributes.id|default('node-' ~ node.id) }}" class="{{ attributes.class}} clearfix"{{ attributes }} role="article">

    This was there before but {{ attributes.class }} (missing space inside curly braces). Also I'd think 'role' would get the default treatment here.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Assigned: Unassigned » joelpittet

#4.1. Too much magic;) Actually AttributeArray doesn't put a space at the beginning. That trick is for attributes only not their values. It avoids classes from always having a space before them class=" space before" and would break that {{ node.id }} example in #4.3 if it did that magic for values too.
#4.2. Good call.
#4.3. Nice catch thanks for the review!

star-szr’s picture

star-szr
he/him
Primary language English
commented

#4.1 - of course, that makes sense. I guess in these cases we can't avoid unnecessary spaces then? class="foo " if attributes.class is empty.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

We could <div class="{{ content_attributes.class|merge(['field-items']) }}">

http://twig.sensiolabs.org/doc/filters/merge.html

Which is what I believe we recommend in one of the twig templates that I saw the other day but can't rememeber which nor can I find it in core yet.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

We cannot use merge... arrayobject's aren't arrays and merge check is_array()
:(

I don't know if we should hack or replace merge filter to do this correctly for Attribute's ArrayObjects?

star-szr’s picture

star-szr
he/him
Primary language English
commented

Or we need some fancier filters/functions to deal with attribute objects? e.g., seems like removing classes is a common need, maybe even removing classes based on a prefix instead of the full class name?

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2187113-10.patch1.98 KB

Banana cleaned a lot of this up, here is a new patch. I have just gone through what's in the original patch so far and I will document some of the more debatable cases I encounter in the issue summary for further discussion/spin off.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: joelpittet » Unassigned
Issue summary: View changes
Issue tags: +TX (Themer Experience), +Needs manual testing
Related issues: +#2108771: Remove special cased title_attributes and content_attributes for Attribute creation
StatusFileSize
new 2187113-11.patch19.31 KB
new interdiff.txt17.33 KB

This patch should be comprehensive now, at first I went through every Twig template looking for issues but then I realized that was silly, so I used this instead:

ack "attributes" --twig --ignore-dir=core/modules/system/tests --ignore-dir=core/vendor core -l | xargs subl

Where subl opens up my editor, and where I have this line in my ~/.ackrc:

--type-set=twig=.twig

I've updated the issue summary to explain what's going on, added beta evaluation, etc.

For safety this should probably get some manual testing, so I also added which templates should be tested to the issue summary. Luckily most of these templates should be fairly easy to find and test!

star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue summary: View changes

Needs more <code> tags.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
StatusFileSize
new Source_of__http___d8_dev_.png23.59 KB
new Source_of__http___d8_dev_.png25.54 KB

Went through each line of the patch and can't fault any one of them. This is a nice cleanup and prevents the possibility duplicates and invalid HTML.

Before:

After:

alexpott’s picture

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

Committed 2affba0 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed a287113 on 8.0.x 
    Issue #2187113 by Cottser, joelpittet: Incorrect usage of attributes in...

Status: Fixed » Closed (fixed)

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