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.

Files: 
CommentFileSizeAuthor
#13 Source_of__http___d8_dev_.png25.54 KBjoelpittet
#13 Source_of__http___d8_dev_.png23.59 KBjoelpittet
#11 interdiff.txt17.33 KBCottser
#11 2187113-11.patch19.31 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,264 pass(es). View
#10 2187113-10.patch1.98 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,194 pass(es). View
#1 2187113-twig-attribute-cleanup-1.patch5.8 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,680 pass(es). View

Comments

joelpittet’s picture

Status: Active » Needs review
FileSize
5.8 KB
PASSED: [[SimpleTest]]: [MySQL] 63,680 pass(es). View

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

joelpittet’s picture

Issue summary: View changes
joelpittet’s picture

Assigned: joelpittet » Unassigned

Unassigning, please review the approach taken.

Cottser’s picture

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

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!

Cottser’s picture

#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

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

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?

Cottser’s picture

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?

Cottser’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,194 pass(es). View

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.

Cottser’s picture

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
FileSize
19.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,264 pass(es). View
17.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!

Cottser’s picture

Issue summary: View changes

Needs more <code> tags.

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
23.59 KB
25.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

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.