Follow-up from #1982024: Lazy-load Attribute objects later in the rendering process only if needed.

Problem/Motivation

We have 3 special attribute variables in preprocess: 'attributes', 'title_attributes', and 'content_attributes'.
When built in preprocess they are created as arrays while all other $variables keys for attributes need to be instantiated as new Attribute(...)

This presents a DX issue where you need to know something about the internals to understand how it works.

There are two things that this does 'for' us that I'm aware:

  1. It makes those un-Attribute()'ed variables easier to merge as arrays (RDF module I believe does this)
  2. Provides a 5.652ms performance gain #1982024-7: Lazy-load Attribute objects later in the rendering process only if needed for wall time. Which could be solved with this #2048637: Add #type => 'attributes' and wrap in Attribute class with drupal_pre_render_attributes

With lazy loading certain $variables keys ('attributes', 'content_attributes', 'title_attributes')


// 
function template_preprocess_something(&$variables) {
  // Special variable keys.
  $variables['attributes'] = array('class' => array('i', 'am', 'lazy', 'and', 'special'));
  $variables['title_attributes'] = array('class' => array('me', 'two'));
  $variables['content_attributes'] = array('title' => "Don't forget me too");
  // The rest of the gang.
  $variables['nested']['attributes'] = new Attribute(array('src' => '/not/special/either.gif'));
  $variables['whatever_attributes'] = new Attribute(array('alt' => 'Un fair:-(');
}

Without lazy loading

function template_preprocess_something(&$variables) {
  // Playing fair and consistant.
  $variables['attributes'] = new Attribute(array('class' => array('i', 'am', 'a', 'work', 'horse')));
  $variables['title_attributes'] = new Attribute(array('class' => array('me', 'two')));
  $variables['content_attributes'] = new Attribute(array('title' => "not as fast but fair is fair."));
  // The rest of the gang.
  $variables['nested']['attributes'] = new Attribute(array('src' => '/img/same.gif'));
  $variables['whatever_attributes'] = new Attribute(array('alt' => 'just like funky and the bunch');
}

Proposed resolution

Remove this chunk of code from the bloated theme() function


  if (!isset($default_attributes)) {
      $default_attributes = new Attribute();
    }
    foreach (array('attributes', 'title_attributes', 'content_attributes') as $key) {
      if (isset($variables[$key]) && !($variables[$key] instanceof Attribute)) {
        if ($variables[$key]) {
          $variables[$key] = new Attribute($variables[$key]);
        }
        else {
          // Create empty attributes.
          $variables[$key] = clone $default_attributes;
        }
      }
    }

And instantiate those attributes when they are needed/used in the templates and not before.

Update

Removing all 3 was challenging in one patch. This patch is now dedicated to only remove title_attributes and content_attributes. A follow-up will be opened to deal with the attributes key after this one is committed.

Remaining tasks

  • Discuss any other drawbacks to removing this.
  • Create a patch
  • Create follow-up issue to finish the job with $variables['attributes']

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#126 interdiff.txt1.17 KBjoelpittet
#126 remove_special_cased-2108771-126.patch8.45 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,161 pass(es). View
#124 interdiff.txt4.93 KBjoelpittet
#124 remove_special_cased-2108771-124.patch9.49 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,169 pass(es). View
#123 interdiff.txt1.73 KBjoelpittet
#122 interdiff-119to122.txt1.11 KBdavidhernandez
#122 remove_special_cased-2108771-122.patch7.01 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,168 pass(es). View
#119 interdiff-116to119.txt478 bytesdavidhernandez
#119 remove_special_cased-2108771-119.patch5.9 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,174 pass(es). View
#116 interdiff-114to116.txt626 bytesdavidhernandez
#116 remove_special_cased-2108771-116.patch5.58 KBdavidhernandez
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,177 pass(es), 2 fail(s), and 0 exception(s). View
#114 remove_special_cased-2108771-112.patch4.97 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,603 pass(es), 3 fail(s), and 0 exception(s). View
#112 remove_some_pointless-2370145-18.patch10.48 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,595 pass(es), 2 fail(s), and 0 exception(s). View
#107 remove_special_cased-2108771-107.patch7.76 KBjjcarrion
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#103 remove_special_cased-2108771-103.patch9.31 KBmikispeed
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove_special_cased-2108771-103.patch. Unable to apply patch. See the log in the details link for more information. View
#97 remove_special_cased-2108771-97.patch8.12 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,888 pass(es), 180 fail(s), and 3,308 exception(s). View
#89 interdiff.txt3.02 KBjoelpittet
#89 2108771-remove-special-variables-keys-89.patch9.42 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,825 pass(es). View
#81 interdiff.txt4.02 KBjoelpittet
#81 2108771-remove-special-variables-keys-81.patch10.19 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,423 pass(es). View
#77 2108771-remove-special-variables-keys-77.patch7.48 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,049 pass(es), 16 fail(s), and 431 exception(s). View
#77 interdiff.txt833 bytesjoelpittet
#75 2108771-remove-special-variables-keys-75.patch7.96 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,981 pass(es), 16 fail(s), and 1,239 exception(s). View
#74 2108771-remove-special-variables-keys-74.patch7.23 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,498 pass(es). View
#74 interdiff.txt3.24 KBjoelpittet
#72 interdiff.txt3.24 KBjoelpittet
#72 2108771-remove-special-variables-keys-72.patch7.17 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,404 pass(es), 38 fail(s), and 2,164 exception(s). View
#67 2108771-remove-special-variables-keys-67.patch7.29 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,877 pass(es). View
#65 2108771-remove-special-variables-keys-65.patch7.23 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,781 pass(es), 50 fail(s), and 2,166 exception(s). View
#56 interdiff.txt954 bytesjoelpittet
#56 2108771-remove-special-variables-keys-56.patch7.24 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,694 pass(es). View
#55 2108771-remove-special-variables-keys-55.patch7.3 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,627 pass(es). View
#54 2108771-remove-special-variables-keys-54-reroll.patch8.82 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,342 pass(es). View
#52 2108771-remove-special-variables-keys-52.patch8.79 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 64,759 pass(es). View
#52 interdiff.txt8.19 KBjoelpittet
#48 2108771-special-attribute-keys-48.patch8.96 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 64,493 pass(es), 231 fail(s), and 0 exception(s). View
#46 2108771-special-attribute-keys-46.patch20.79 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 62,383 pass(es), 567 fail(s), and 416,178 exception(s). View
#42 interdiff.txt568 bytesjoelpittet
#42 2108771-special-attribute-keys-42.patch9.11 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,412 pass(es). View
#40 2108771-special-attribute-keys-40-reroll.patch9.11 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/block/block.module. View
#38 2108771-special-attribute-keys-38.patch20.97 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 62,083 pass(es), 259 fail(s), and 337,697 exception(s). View
#36 interdiff.txt2.18 KBjoelpittet
#36 2108771-special-attribute-keys-35.patch9.8 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 63,678 pass(es). View
#33 interdiff.txt596 bytesjoelpittet
#33 2108771-special-attribute-keys-33.patch9.41 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 63,678 pass(es), 2 fail(s), and 0 exception(s). View
#29 2108771-special-attribute-keys-29.patch9.86 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#27 2108771-special-attribute-keys-27.patch20.6 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 63,232 pass(es), 118 fail(s), and 1,778 exception(s). View
#24 interdiff.txt1.27 KBjoelpittet
#24 2108771-special-attribute-keys-23.patch17.32 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 62,980 pass(es), 43 fail(s), and 7,568 exception(s). View
#23 interdiff.txt16.14 KBjoelpittet
#23 2108771-special-attribute-keys-23.patch17.32 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 63,042 pass(es), 60 fail(s), and 8,350 exception(s). View
#20 2108771-special-attribute-keys-reroll-20.patch17.01 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 62,620 pass(es), 137 fail(s), and 25,654 exception(s). View
#15 2108771-15-special-attribute-keys.patch19 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 58,840 pass(es), 128 fail(s), and 9,445 exception(s). View
#8 2108771-8-special-attribute-keys.patch20.01 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 58,890 pass(es), 129 fail(s), and 20,803 exception(s). View

Comments

joelpittet’s picture

webchick’s picture

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.

webchick’s picture

Issue summary: View changes

Updated issue summary.

Fabianx’s picture

Issue tags: -Performance

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.

Fabianx’s picture

Issue tags: +Performance

This is related to performance ...

joelpittet’s picture

Issue tags: +Performance

@Fabianx, having a #type'd array for attributes would be more consistent and very likely faster.

My main concern is like you hinted in the "EDIT". Though my counter argument there is that even though it is an 'internal optimization' people are looking to the preprocess layer and templates as a way to get hints on how to do things in their themes. This provides unnecessary confusion as it is now with 3 key'd variables getting treated differently than the rest. So it is front facing.

#type=>attributes is an issue #2048637: Add #type => 'attributes' and wrap in Attribute class with drupal_pre_render_attributes and at first I was confused about what that was, then after you explained it so well in #3 it makes more sense. Personally I'd still like to be able to manipulate the Attribute object at twig time but using the following would make it easier to merge and manipulate and possibly be a better DX win.

// Without lazy loading 3 magic attribute keys & #type'd
function template_preprocess_something(&$variables) {
  // Playing fair and consistant.
  $variables['attributes'] = array(
    '#type' => 'attributes',
    'class' => array('i', 'am', 'a', 'work', 'horse'))
  );
  $variables['title_attributes'] = array(
    '#type' => 'attributes',
    'class' => array('me', 'two'))
  );
  $variables['content_attributes'] = array(
    '#type' => 'attributes',
    'title' => "not as fast but fair is fair.")
  );
  // The rest of the gang.
  $variables['nested']['attributes'] = array(
    '#type' => 'attributes',
    'src' => '/img/same.gif')
  );
  $variables['whatever_attributes'] = array(
    '#type' => 'attributes',
    'alt' => 'just like funky and the bunch',
  );
}

FYI this issue would still be relevant in removing those magic keys:)

joelpittet’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Title: Remove Lazy-load Attribute in the name of DX » Remove special $variables keys for Attribute object creation in the name of DX

Changing the title on this because it's not the lazy-loading I want to remove, it's the special keys.

joelpittet’s picture

Issue summary: View changes

Updated issue summary.

jenlampton’s picture

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.

joelpittet’s picture

Status: Active » Needs review
FileSize
20.01 KB
FAILED: [[SimpleTest]]: [MySQL] 58,890 pass(es), 129 fail(s), and 20,803 exception(s). View

I very highly doubt this will pass all the tests... but here's a rough to see how much effort this issue will be.

Status: Needs review » Needs work

The last submitted patch, 2108771-8-special-attribute-keys.patch, failed testing.

webchick’s picture

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

joelpittet’s picture

I could do more, look how many tests are run, room for 'improvement'

Cottser’s picture

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.

joelpittet’s picture

Yeah that one has some nice 917 fail(s), and 1,588 exceptions.

And less code to fail that little:) The #type => attributes will work but only if we find a way to either convert early without the recusive $variables search, OR convert late but replace the array with a TwigReference on the fly when key's are accessed in {{ twig.attributes.key }} get's printed.

c4rl’s picture

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.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
19 KB
FAILED: [[SimpleTest]]: [MySQL] 58,840 pass(es), 128 fail(s), and 9,445 exception(s). View

A re-roll and add the prepped empty arrays back in. Should have less exceptions.

Status: Needs review » Needs work

The last submitted patch, 15: 2108771-15-special-attribute-keys.patch, failed testing.

joelpittet’s picture

Just want to say +1 for #14 Thanks c4rl that is my sentiment exactly. Also, the only reason this Attribute class really *needs* to exist is for the handy little #printed magic it does for <tag class="{{ attributes.class }}"{{ attributes }}> which, if this got in: #2114563: Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions. could be replicated with <tag class="{{ attributes.class }}"{{ attributes|hide('class') }}>...

rteijeiro’s picture

Issue summary: View changes
joelpittet’s picture

Assigned: Unassigned » joelpittet

Picking this back up.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
17.01 KB
FAILED: [[SimpleTest]]: [MySQL] 62,620 pass(es), 137 fail(s), and 25,654 exception(s). View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 20: 2108771-special-attribute-keys-reroll-20.patch, failed testing.

The last submitted patch, 20: 2108771-special-attribute-keys-reroll-20.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
17.32 KB
FAILED: [[SimpleTest]]: [MySQL] 63,042 pass(es), 60 fail(s), and 8,350 exception(s). View
16.14 KB

remove a few more exceptions and such.

joelpittet’s picture

FileSize
17.32 KB
FAILED: [[SimpleTest]]: [MySQL] 62,980 pass(es), 43 fail(s), and 7,568 exception(s). View
1.27 KB

The last submitted patch, 23: 2108771-special-attribute-keys-23.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 24: 2108771-special-attribute-keys-23.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
20.6 KB
FAILED: [[SimpleTest]]: [MySQL] 63,232 pass(es), 118 fail(s), and 1,778 exception(s). View

This will cut down a good chunk of exceptions *crosses fingers* more than half?

Status: Needs review » Needs work

The last submitted patch, 27: 2108771-special-attribute-keys-27.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.86 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Ok I'm simplifying this because there is too much engrained. This is a compromise patch, removing title_attributes and content_attributes special variables. Also would be nicer to deal with if there wasn't 'render element'...

Status: Needs review » Needs work

The last submitted patch, 29: 2108771-special-attribute-keys-29.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 29: 2108771-special-attribute-keys-29.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.41 KB
FAILED: [[SimpleTest]]: [MySQL] 63,678 pass(es), 2 fail(s), and 0 exception(s). View
596 bytes

Not sure the deal there... but undoing the form.inc change.

Status: Needs review » Needs work

The last submitted patch, 33: 2108771-special-attribute-keys-33.patch, failed testing.

joelpittet’s picture

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.8 KB
PASSED: [[SimpleTest]]: [MySQL] 63,678 pass(es). View
2.18 KB

This should fix those last 3...

The last submitted patch, 33: 2108771-special-attribute-keys-33.patch, failed testing.

joelpittet’s picture

Assigned: joelpittet » Unassigned
FileSize
20.97 KB
FAILED: [[SimpleTest]]: [MySQL] 62,083 pass(es), 259 fail(s), and 337,697 exception(s). View

Re-roll. This needs review:)

Status: Needs review » Needs work

The last submitted patch, 38: 2108771-special-attribute-keys-38.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.11 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/block/block.module. View

Whoops, wrong patch! That has both 'attributes' and the other two. I'm aiming for just the other two...

Status: Needs review » Needs work

The last submitted patch, 40: 2108771-special-attribute-keys-40-reroll.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
9.11 KB
PASSED: [[SimpleTest]]: [MySQL] 63,412 pass(es). View
568 bytes

Fixed typo

joelpittet’s picture

+++ b/core/modules/block/block.module
@@ -490,6 +491,15 @@ function template_preprocess_block(&$variables) {
+  static $default_attributes;
+  if (!isset($default_attributes)) {
+    $default_attributes = new Attribute;
+  }
+
+  // For best performance, we only instantiate Attribute objects when needed.
+  $variables['title_attributes'] = isset($variables['title_attributes']) ? new Attribute($variables['title_attributes']) : clone $default_attributes;
+  $variables['content_attributes'] = isset($variables['content_attributes']) ? new Attribute($variables['content_attributes']) : clone $default_attributes;

This may be premature optimisation. Thoughts?

joelpittet’s picture

Issue tags: +focus

Adding tag.

Cottser’s picture

Issue tags: -focus +sprint

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.

joelpittet’s picture

FileSize
20.79 KB
FAILED: [[SimpleTest]]: [MySQL] 62,383 pass(es), 567 fail(s), and 416,178 exception(s). View

Re-roll.

Status: Needs review » Needs work

The last submitted patch, 46: 2108771-special-attribute-keys-46.patch, failed testing.

joelpittet’s picture

FileSize
8.96 KB
FAILED: [[SimpleTest]]: [MySQL] 64,493 pass(es), 231 fail(s), and 0 exception(s). View

Ok second time I've done that. This is the right patch with only the title_attributes and content_attributes not automagically converted to Attribute objects.

Going to actually get this in my repo so it doesn't happen again...

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 48: 2108771-special-attribute-keys-48.patch, failed testing.

joelpittet’s picture

This should be easier to fix up than 400K exceptions;)

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.19 KB
8.79 KB
PASSED: [[SimpleTest]]: [MySQL] 64,759 pass(es). View
Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
8.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,342 pass(es). View

Re-rolled.

joelpittet’s picture

FileSize
7.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,627 pass(es). View

Let's see were this is at.

Again this isn't everything but it's a big step forward to removing the special case and the confusion between what is an array in preprocess and what is an Attribute object. This still leaves $variables['attributes'] as an array, but makes content_attributes and title_attributes act like all the other attributes in the system.

joelpittet’s picture

FileSize
7.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,694 pass(es). View
954 bytes

Small little consistency change from @davereid. Going to performance test this.

joelpittet’s picture

whoops.

joelpittet’s picture

re #57 so wow, I didn't expect a ~3% performance gain when when the reason they are like that to start was for performance... maybe I need someone else to double check?

joelpittet’s picture

Second set of results just to see not a fluke.
=== 8.x..8.x compared (538a638f2c760..538a642c85962):

ct : 75,451|75,451|0|0.0%
wt : 602,296|614,504|12,208|2.0%
cpu : 594,238|606,226|11,988|2.0%
mu : 45,144,536|45,144,536|0|0.0%
pmu : 45,219,424|45,219,424|0|0.0%

Profiler output

=== 8.x..2108771-remove-special-variables-keys-56 compared (538a638f2c760..538a6499b782f):

ct : 75,451|75,385|-66|-0.1%
wt : 602,296|611,801|9,505|1.6%
cpu : 594,238|603,185|8,947|1.5%
mu : 45,144,536|45,142,960|-1,576|-0.0%
pmu : 45,219,424|45,216,008|-3,416|-0.0%

Profiler output

Dave Reid’s picture

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

benjifisher’s picture

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&...

Cottser’s picture

Title: Remove special $variables keys for Attribute object creation in the name of DX » Remove special casing for title_attributes and content_attributes with regards to Attribute object creation
Issue tags: +Needs change record

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.

joelpittet’s picture

Title: Remove special casing for title_attributes and content_attributes with regards to Attribute object creation » Remove special cased title_attributes and content_attributes for Attribute creation
msonnabaum’s picture

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.

joelpittet’s picture

FileSize
7.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,781 pass(es), 50 fail(s), and 2,166 exception(s). View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 65: 2108771-remove-special-variables-keys-65.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
7.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,877 pass(es). View

Hmm, well that re-roll needed some tweak.

joelpittet’s picture

50 nodes in the front page view with bartik.
Sidebars added for recent comments and recent content as well as who online block.

Pretty negligible.

=== 8.x..8.x compared (5394e21e9bd45..5394e2ae29e1b):

ct  : 200,177|200,177|0|0.0%
wt  : 1,029,973|1,033,401|3,428|0.3%
cpu : 1,017,925|1,021,761|3,836|0.4%
mu  : 48,340,880|48,340,880|0|0.0%
pmu : 48,613,040|48,613,040|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5394e21e9bd45&...

=== 8.x..2108771-remove-special-variables-keys-56 compared (5394e21e9bd45..5394e33e8d847):

ct  : 200,177|200,020|-157|-0.1%
wt  : 1,029,973|1,030,407|434|0.0%
cpu : 1,017,925|1,018,833|908|0.1%
mu  : 48,340,880|48,340,616|-264|-0.0%
pmu : 48,613,040|48,612,096|-944|-0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5394e21e9bd45&...

joelpittet’s picture

Issue summary: View changes
Cottser’s picture

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?

joelpittet’s picture

Issue summary: View changes

Thanks @Cottser, it should be consistent through-in-through. Thanks for reviewing and noticing. Also the template should expect that this variable is one type of thing, an Attribute object... sometimes. So I'll change this and also retest.

I've added a note about the followup to the Proposed Resolution of the issue summary.

joelpittet’s picture

FileSize
7.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,404 pass(es), 38 fail(s), and 2,164 exception(s). View
3.24 KB

Hmm sorry I did a combined re-roll and the change in #70 The interdiff is fairly useless because I missed something in the reroll as some aria stuff got dropped and re-added.

Status: Needs review » Needs work

The last submitted patch, 72: 2108771-remove-special-variables-keys-72.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
3.24 KB
7.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,498 pass(es). View

Ah that failed because we can no longer assume content_attributes exists before because the content class was removed.

joelpittet’s picture

FileSize
7.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,981 pass(es), 16 fail(s), and 1,239 exception(s). View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 75: 2108771-remove-special-variables-keys-75.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
833 bytes
7.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,049 pass(es), 16 fail(s), and 431 exception(s). View

Fix the comment field

Status: Needs review » Needs work

The last submitted patch, 77: 2108771-remove-special-variables-keys-77.patch, failed testing.

joelpittet’s picture

Status: Needs work » Postponed

Those error could be fixed but they would be easier to look at once this is in: #2285451: Create addClass() and removeClass() methods on Attribute object for merging css class names.

benjifisher’s picture

Status: Postponed » Needs work

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
10.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,423 pass(es). View
4.02 KB

Let's see how testbot likes this!

Fabianx’s picture

Status: Needs review » Needs work
  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.

joelpittet’s picture

@Fabianx It hasn't seemed to be a "performance nightmare" in my recent profiling on this issue. @see #68

This original issue was to remove all the late wrapping of arrays to Attribute objects. That was a daunting task and I skirted around 'attributes' key because it's everywhere, though the goal is still to make those consistent as well and not sometimes (in preprocess) arrays, and sometimes (in preprocess and templates) Attribute objects.

So it's only a 10K patch, maybe I need to finish the job here instead of splitting it into to jobs. Also pondering if I can some how instantiate the empty attribute object in the hook_theme, but not sure if I can with 'render elements' yet but I'll give that a try too so we aren't defining their type so late in the processes.

And this is loosely related to the banana plan of getting classes out of the preprocess. #2289511: [meta] Results of Drupalcon Austin's Consensus Banana

Fabianx’s picture

Status: Needs work » Needs review
Issue tags: -Performance

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.

joelpittet’s picture

@Fabianx I don't mean to offend you if I have, it's hard to read sarcasm in emails/comments though I hope it's not that and this is seriously ok?

I did the performance test with render cache turned off (before that NULL cache bin was broken in head).

I really don't want to introduce a performance bottle neck that's why I took the time to xhprof this change to the best of my ability. I do plan to do another performance after the 'attributes' key is an object as well, though if the test scenario or strategy is useless I don't want to spend X hours setting it up and running for nothing?

Caching is a great way to increase performance by a heap load but it seems to mask the hell out of real bottlenecks in code. So turning cache off (or turning off the right cache for the test) is key(quite sure you taught me that;)

Fabianx’s picture

#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!

joelpittet’s picture

@Fabianx thank you, that is very clear! I'll continue to bench(also because I'm curious of notable differences).

The biggest DX issue that addClass() seems to clean up is that ArrayAccess leads people to think that Attribute is an array. And leads to people blindly doing

$variables['attributes']['class'][] = 'new-class';

And without initializing that class key as an array, you get a nasty error of "indirect modification blah blah blah".
Which you wouldn't get with a normal array but since this fixes but clobbers any class property from being passed into the render array it becomes a catch 22:

$variables['attributes']['class'] = array(); // Fixed and clobbered.
$variables['attributes']['class'][] = 'new-class';

Which leads to:

if (!isset($variables['attributes']['class'])) {
  $variables['attributes']['class'] = array(); 
}
$variables['attributes']['class'][] = 'new-class'; // Fixed if class was instance of AttributeArray before if statement.

So treating Attribute as an object has some nice DX improvements that solve this extra thinking of "Is this an Array or Attribute object? Is the 'class' AttributeArray created?"

Still very curious though if I can instantiate it earlier if the theme/template expects that variable(in hook_theme()). Though 'render element' doesn't provide much of a 'contract' of what the template expects for variables.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Bump, this needs a reroll.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
9.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,825 pass(es). View
3.02 KB

Re-roll and added back the cloning $default_attributes for now. Would like to go full tilt on this for attributes but it's a daunting task and would likely break other issues into rerolls. This however, with just title_attributes and content_attributes, is relatively small.

Maybe we can put this in as-is? And work on the big kahuna key 'attributes' after in a follow up?

Cottser’s picture

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?

joelpittet’s picture

@Cottser currently we are using ArrayAccess to modify other attributes. Classes because they are stored as an array are a bit tricky and that's why addClass/removeClass works well because it ensures the array exists as I was trying to demo in #87.

I believe there is another issue around for .setAttribute()/.removeAttribute() discussion.

ArrayAccess fails when it's nested array like objects and the first one dictates the second.

$attr = new Attribute();
$attr['id'] = 'no problem';
$attr['class'][] = 'fail';
$attr['class'] = array('no problem');
Fabianx’s picture

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?

Cottser’s picture

Yeah that thought crossed my mind as well :)

joelpittet’s picture

@Fabianx and @Cottser, I did attempt to do that in one or two issues earlier. Though I didn't think to actually pre instantiate which is an interesting idea though never crossed my mind. I looked at ways to check if the first key coming in is class and special case it (even though there is one or two RDF attributes that use Attribute Array as well).

Though the addClass() merges, which should clean up in preprocess likely or at least help the DeepMerging that happens in come preprocesses. Also we had some instances of duplicate classes, which also doesn't happen anymore, and in the template there is no extra spaces because the method acts on the object and we don't have to use |without to meet our needs. (Reason for that is that internally Twig merge check is_array() and ArrayAccess is not an array and doesn't merge like Arrays, and I want to avoid hacking core, twig core, at all costs).

So I think "solved" needs by instanciating it, may have solved one issue I think addClass() method solved much more. Not to mention it looks like jQuery and for themers that is a plus(I think that was along the lines of your comment in another issue regarding adding addClass, IIRC @Fabianx)

Fabianx’s picture

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

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

joelpittet’s picture

@Fabianx I was thinking we should, but naming conventions could be tricky. FYI That discussion is happening here: #2325517: Add methods for adding/removing attributes (not classes) on Attribute objects

joelpittet’s picture

FileSize
8.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,888 pass(es), 180 fail(s), and 3,308 exception(s). View

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 97: remove_special_cased-2108771-97.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 97: remove_special_cased-2108771-97.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 97: remove_special_cased-2108771-97.patch, failed testing.

mikispeed’s picture

FileSize
9.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove_special_cased-2108771-103.patch. Unable to apply patch. See the log in the details link for more information. View

Re-rolled.

lauriii’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 103: remove_special_cased-2108771-103.patch, failed testing.

davidhernandez’s picture

Issue tags: +Needs reroll
jjcarrion’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

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';

Status: Needs review » Needs work

The last submitted patch, 107: remove_special_cased-2108771-107.patch, failed testing.

lauriii’s picture

For me lates patch caused forever loop/php timeout

Cottser’s picture

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

joelpittet’s picture

Too cool for school...

Actually if we remove the item_attributes piece that isn't in scope this is fairly small patch.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
10.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,595 pass(es), 2 fail(s), and 0 exception(s). View

Here's a re-roll without the field_attributes changes.

Things got a bit simpler/more consistant since classy moved classes out of core! Big win there:)

Status: Needs review » Needs work

The last submitted patch, 112: remove_some_pointless-2370145-18.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,603 pass(es), 3 fail(s), and 0 exception(s). View

Whoops wrong remove-...*.patch.

Status: Needs review » Needs work

The last submitted patch, 114: remove_special_cased-2108771-112.patch, failed testing.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
5.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,177 pass(es), 2 fail(s), and 0 exception(s). View
626 bytes

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?

Status: Needs review » Needs work

The last submitted patch, 116: remove_special_cased-2108771-116.patch, failed testing.

joelpittet’s picture

@davidhernandez thanks, that knocked back one failure. The date field seems to be getting it's datetime wrapper required classes in some weird way, looking to track it down.

To test it, just create a 'required' date field on the article type and see the field is missing the h4 classes.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
5.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,174 pass(es). View
478 bytes

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.

joelpittet’s picture

Nice! I spent a bunch more time last night trying to solve the problem thinking it was more systemic:S

Cottser’s picture

Status: Needs review » Needs work

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

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
7.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,168 pass(es). View
1.11 KB

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

joelpittet’s picture

FileSize
1.73 KB

I'd like to do this for all of them... (see interdiff) but it only works for variables and all the rest are 'render element'. Boo to 'render element'!

It looks cleaner and may be even nicer/performant with a Attribute::factory() returning empty default clones.

I wonder what effort would be involved in converting all 'render element' to variables. I've seen it done but it would be easier if variables wasn't so strict on requiring the variables be defined ahead of time. I digress...

Anyways, thoughts?

joelpittet’s picture

FileSize
9.49 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,169 pass(es). View
4.93 KB

I discussed with @davidhernandez in IRC and I think we are of the same mindset.

A) It's weird we have to do this attribute check and build in the preprocess.
B) No need for preprocess in core (except for tests because we want it to still work).

One blocker for this desired outcome is 'render element'.

But here's what that would look like with datetime_wrapper converted to variables! What a clean preprocess because all the variables map to what the template expects!

davidhernandez’s picture

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

joelpittet’s picture

FileSize
8.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,161 pass(es). View
1.17 KB
+++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
@@ -28,7 +29,9 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
-    $element['#theme_wrappers'][] = 'datetime_wrapper';
+    $element['#theme_wrappers']['datetime_wrapper'] = [
+      '#title_attributes' => new Attribute(),
+    ];

Whoops that was my first attempt before moving hook_theme to variables. Not needed.

The other 'render element's are hard/impossible to change at the moment because they take loop through the element_children() or have variable variables.

joelpittet’s picture

Kinda off topic but #2465399-18: IGNORE: Patch testing issue would be nice and it's green. Allows for variables to be passed through that are not explicitly defined in the hook_theme(). Just a bit less rigid. Food for thought

joelpittet’s picture

Version: 8.0.x-dev » 9.x-dev
Status: Needs review » Postponed

Bumping this 9.x to triage since we are in RC for 8.0.x

fgm’s picture

Version: 9.x-dev » 8.2.x-dev
Status: Postponed » Active

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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.