Remove special cased title_attributes and content_attributes for Attribute creation

Coming in as a layperson to all of this, the proposed "explicitly load whenever needed consistently" way definitely makes more logical sense to me. But I'd love to hear from FabianX and steveoliver and anyone else involved at the original issue.

Updated issue summary.

I don't think it is a good idea, to do it.

In my book, I even would propose to #type those Attribute arrays.

The early instantiation is very unnecessary and the performance is one magic call per array access.

Again the problem is less the magic access, but that people "think" it is fast to use, which is much larger DX problem.

I think removing __set from it could work and making them read-only; instantiate once in the end.

Attribute objects are only useful at the presentation layer for output so that you can print the class first and the rest later.

Again, I think the real DX win is if we have "lightly" typed data in our theme layer that defines that these variables are "attributes".

Because in the end you will need to know in a template if something is an attribute or not, so it needs to happen in a late preprocess / prepare step anyway to convert to new Attribute().

And those magic classes are nothing new, they existed as the much more confusing and less consistent classes_array already in D7 ...

---

EDIT:

Also the current code perfectly works when instantiating the Attribute() objects early. So this could be thought of as an internal optimization as well and we will likely need more internal optimizations rather than less.

This is related to performance ...

Most templates don't need all three of these variables, so I'd love to remove them :)

Another thing to note here is that these keys are also incorrectly named. By the time we get to the template we should be printing {{ title.attributes }} and {{ content.attributes }} not title_attributes and content_attributes. That means these attributes should really be attached to the thing they are describing (title and/or content) and not free-floating. To make things make sense, what we need is correctly structured data.

Removing these here means we can add attributes where they are actually needed: in the correct places - and with the correct names.

That's an impressive number of exceptions, sir! :)

Just reverting #1982024: Lazy-load Attribute objects later in the rendering process only if needed is too much of a DX (difficult to manipulate or merge in attributes later in the preprocess/prepare stack) and performance regression IMO.

In general I would love to remove the special casing we have though, and #2048637: Add #type => 'attributes' and wrap in Attribute class with drupal_pre_render_attributes seems like a viable way forward at first glance.

First of all, let's clarify that the *relative* performance gain in #1982024-7: Lazy-load Attribute objects later in the rendering process only if needed was less than 1%.

IMHO, the motivating factor of #1982024: Lazy-load Attribute objects later in the rendering process only if needed is an anti-pattern: Attribute() objects should be instantiated as such from the start, period. If they aren't used, is that such a problem? Are we really that afraid of instantiating objects? By having this fear, we create other problems.

We are only obscuring the fact that Attribute() objects are special things by putting them in some sort of '#type', which works now completely differently than other things that have '#type.'

For the sake of true consistency: I would much prefer to have Attribute objects always be Attribute objects and not arrays that are later converted.

Updated issue summary.

Changing focus tag to sprint, that is the convention other initiatives have been using and is how I had set up http://drupaltwig.org/issues/focus.

Tagging for reroll.

This looks good to me, but could maybe use someone more ingrained in the the theme system in D8 to give a final OK.

I created a node with three custom blocks and four comments. Before applying the patch, I see this markup in one of my comments:

<h3 property="schema:name" datatype=""><a href="/comment/1" class="permalink" rel="bookmark">Proverb #1</a></h3>
<div class="content">
  <span resource="/node/2" class="rdf-meta"></span>
  <div class="field field-comment--comment-body field-name-comment-body field-type-text-long field-label-hidden quickedit-processed" data-quickedit-field-id="comment/1/comment_body/en/full">
    <div class="field-items">
      <div class="field-item" property="schema:text"><p>In seed time learn, in harvest teach, in winter enjoy.</p></div>
    </div>
  </div>
</div>
<!-- /.content -->

Looking at comment.html.twig in the Bartik theme, it looks as though the attributes on the h3 element are the title attributes and the class on the outer div is the content attribute.

After applying the patch, I see exactly the same markup:

<h3 property="schema:name" datatype=""><a href="/comment/1" class="permalink" rel="bookmark">Proverb #1</a></h3>
<div class="content">
  <span resource="/node/2" class="rdf-meta"></span>
  <div class="field field-comment--comment-body field-name-comment-body field-type-text-long field-label-hidden quickedit-processed" data-quickedit-field-id="comment/1/comment_body/en/full">
    <div class="field-items">
      <div class="field-item" property="schema:text"><p>In seed time learn, in harvest teach, in winter enjoy.</p></div>
    </div>
  </div>
</div>
<!-- /.content -->

When profiling, I see (if I am reading this correctly) a performance improvement after applying the patch, although the difference is small enough that it might be random. The important difference is "ct": is that the total number of function calls while building the page, or is it finer than that?

=== 8.x..8.x compared (53923d861ff11..53923e3238612):

ct  : 157,637|157,637|0|0.0%
wt  : 936,353|935,884|-469|-0.1%
cpu : 863,921|863,602|-319|-0.0%
mu  : 15,336,708|15,336,708|0|0.0%
pmu : 15,466,252|15,466,252|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53923d861ff11&...

=== 8.x..2108771_56_attributes compared (53923d861ff11..53923e49d2b58):

ct  : 157,637|157,386|-251|-0.2%
wt  : 936,353|934,787|-1,566|-0.2%
cpu : 863,921|862,829|-1,092|-0.1%
mu  : 15,336,708|15,335,836|-872|-0.0%
pmu : 15,466,252|15,465,556|-696|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53923d861ff11&...

Patch looks great to me. This will need a small change record before commit, updating the issue summary to indicate this is only affecting title_attributes and content_attributes, not the main 'attributes', would be a great step in the right direction.

Just went over this with joelpittet and I dont think there's a performance difference here worth considering. Certainly not enough to outweigh the DX improvement.

So the issue summary could use an update to explain that this is only changing title_attributes and content_attributes I think.

One more question:

+++ b/core/modules/block/block.module
@@ -382,6 +383,10 @@ function template_preprocess_block(&$variables) {
+  // For best performance, we only instantiate Attribute objects when needed.
+  $variables['title_attributes'] = isset($variables['title_attributes']) ? new Attribute($variables['title_attributes']) : NULL;
+  $variables['content_attributes'] = isset($variables['content_attributes']) ? new Attribute($variables['content_attributes']) : NULL;

+++ b/core/modules/node/node.module
@@ -694,6 +694,9 @@ function template_preprocess_node(&$variables) {
+
+  $variables['content_attributes'] = isset($variables['content_attributes']) ? new Attribute($variables['content_attributes']) : new Attribute();
+  $variables['title_attributes'] = isset($variables['title_attributes']) ? new Attribute($variables['title_attributes']) : new Attribute();

Why does block use NULL and everything else use new Attribute()? I know this one is the only one with a performance comment but then why aren't all the other ones using the same pattern?

It looks as though #2285451: Create addClass() and removeClass() methods on Attribute object for merging css class names. has been committed, so I am moving this back to NW.

1. +++ b/core/includes/theme.inc
   @@ -623,19 +621,8 @@ function _theme($hook, $variables = array()) {
   -    if (!isset($default_attributes)) {
   -      $default_attributes = new Attribute();
   -    }
   ...
   -        else {
   -          // Create empty attributes.
   -          $variables[$key] = clone $default_attributes;
   -        }
   ...
   +      $variables['attributes'] = new Attribute($variables['attributes']);
        }
   

   Can we please keep the default implementation of using clone for $default_attributes when its empty?

   The optimization is still worth it though it could look like

   <?php

   if (isset($variables['attributes']) && !($variables['attributes'] instanceof Attribute)) {

   if ($variables['attributes']) {
   $variables['attributes'] = new Attribute($variables['attributes']);
   } else {
   if (!isset($empty_attributes)) {
   $empty_attributes = new Attribute();
   }

   $variables['attributes'] = clone $empty_attributes;
   }
   }

   And _theme is really much critical render path.

2. +++ b/core/includes/theme.inc
   @@ -2418,11 +2401,6 @@ function template_preprocess_field(&$variables, $hook) {
   -  static $default_attributes;
   -  if (!isset($default_attributes)) {
   -    $default_attributes = new Attribute;
   -  }
   

   Again, lets please keep this.

   clone is way faster than new and the added complexity is not much ...

   Can also use if() and create on the fly.

3. +++ b/core/includes/theme.inc
   @@ -2431,8 +2409,11 @@ function template_preprocess_field(&$variables, $hook) {
   +  $variables['title_attributes'] = isset($variables['title_attributes']) ? new Attribute($variables['title_attributes']) : new Attribute();
   +  $variables['content_attributes'] = isset($variables['content_attributes']) ? new Attribute($variables['content_attributes']) : new Attribute();
   

   Especially if you add even more new Attribute() calls.

4. +++ b/core/modules/block/block.module
   @@ -293,6 +291,11 @@ function template_preprocess_block(&$variables) {
   +  $variables['title_attributes'] = isset($variables['title_attributes']) ? new Attribute($variables['title_attributes']) : new Attribute();
   

   Block is more seldomly called, so okay with not optimizing here.

5. +++ b/core/modules/comment/comment.module
   @@ -1020,7 +1022,8 @@ function comment_preprocess_field(&$variables) {
   -    $variables['content_attributes']['class'] = array('title', 'comment-form__title');
   +    $variables['title_attributes']->addClass('title');
   +    $variables['content_attributes']->addClass(array('title', 'comment-form__title'));
   

   This is a bad idea, because now content_attributes, etc. is special, while attributes is not.

   What if I write:

   $vars['attributes']->addClass()

   which is a performance nightmare obviously.

Overall nice, but this introduces other special case (ugh) and reduces performance by removing clone's.

Lets not do that ...

EDIT:

On the benchmarks, with many many fields the performance is still seeable for clone vs. new() and it really does not hurt, while I don't care at all about content_attributes or title_attributes.

Okay, removing performance tag. I am officially giving up on that kind of optimizations hereby.

Lets move everything to objects and deal with the issue by adding more and extensive render caching and using PHP-VM, etc.

Performance (in raw page speed generation time) and Drupal 8 just don't match and that is okay, but lets not say we do ...

We have better strategies to deal with that ...

And I am happy about that cause it means we can cleanup some more things to be objects, etc. without worrying about it and concentrate on the really important things like DX and improved caching.

#85:

I really appreciate you taking the time to benchmark things, but the rest of core does not and all the convert X to service issues all add up a little and way more.

My little 'use clone instead of new' optimization is useless compared to the performance hit we are taking elsewhere in core.

This issue taught me that now.

And while it is definitely great to benchmark the attributes to class one again before doing the changes, this should not hold it up - as long as the rest of core is not benchmarked in the same way as the DX win is too great for consistency, etc.

A class based structure will always be slower, but if we call node->getField()->getIterator()->getWhatever and other chained things frequently, we can as well call $attributes->addClass().

We just need to stop saying the theme system is different in terms of performance or in the render chain:

No, it is not. If others in core (in every other issue) don't care about performance, I do not either and all I want is equality in that.

The theme system benchmarks only makes it clearer that core gets slower and slower, but that is not due to the theme system itself, but elsewhere.

The render / theme chain is not special and should not be treated special. That is all my frustration with that.

Lets keep up the great benchmarks, but just don't worry about performance for the moment so much and especially not little micro things like clone vs. new, etc. but on clean OO architecture. Yes, its slower, but that battle has long been lost already and with PHP-VM / PHP 6 things will get way faster in the future, so this is like:

assembly vs. OOP - yes assembly is faster, but no you don't wanna write it.

So totally agree with removing all the special casing :).

Thanks for that! Keep up the great DX!

Bump, this needs a reroll.

So are we saying addClass/removeClass is the way to go for any non-template_preprocess function modifying classes? What about modifying other attributes, does that work?

We _could_ have solved that by internally always populating the class key with an empty array on Attribute creation though, because we check for empty class IIRC ...

Would that not be cleaner, then having two ways ArrayAccess or addClass to set things on attribute objects?

Yeah that thought crossed my mind as well :)

I am okay with it, just wanted to know the reasoning.

Should we add an attr() method, too? To be consistent?

Re-rolled.

I have made the re-roll from the #97 (the #103 didn't apply).

In the patch #97 there was a change in the way to add the class in the search.module.

- $variables['attributes']['class'][] = 'container-inline';
+ $variables['attributes']->addClass('container-inline');

I have removed that change because otherwise it breaks the whole site. I have left it like this:
$variables['attributes']['class'][] = 'container-inline';

For me lates patch caused forever loop/php timeout

What are we thinking about this one now folks? Disruptive? Not too disruptive?

We still need the attributes for node, correct? Those disappeared at some point.

Also, there is a fail for Drupal\datetime\Tests\DateTimeFieldTest which makes no sense. I can't actually get this test to pass locally, with or without the patch. I checked previous commits and still can't get it to pass. The date format is correct, but the date is off by one day. Is there some odd environment issue, or something timezone related?

For me, the issue isn't with the markup. I'm getting local failures on the date value itself. It's annoying, but I'll upload what I have to see if testbot at least is happier.

Also, I assume every occurrence should be accounted for then? There are other places where title_attributes and content_attributes appear, like aggregator.

Sounds like more work is needed then based on #119.

Looks like aggregator might have been the only thing left. Everything else looks covered, mostly by the field change.

You also have the search_theme() change in there. :)

I guess we could target it for 8.2.x if we keep title|content_attributes marked as deprecated and introducing the new mechanism: this seems to be what minor versions like 8.*.x are about.