Part of meta-issue #1788918: [GTD] [META] Prepare for Twig main engine core inclusion until December 1

template_preprocess() provides 4 arays to assist with theming content:

attributes_array
classes_array
title_attributes_array
content_attributes_array

I am not sure why classes_array exists, and why it isn't incorporated into attributes_array[]class]. Alternatively, I don't know why title_attributes_array and content_attributes_array don't have title_classes_array and content_classes_array pairs.

It seems that things would be easier to use, and more understandable, if there was consistency here.

CommentFileSizeAuthor
#91 drupal-1290694-91.patch87.25 KBtstoeckler
#91 interdiff-87-91.txt1.13 KBtstoeckler
#88 drupal-1290694-88.patch87.21 KBtstoeckler
#88 interdiff-87-88.txt1.08 KBtstoeckler
#87 drupal-1290694-87.patch87.23 KBtim.plunkett
#87 interdiff.txt596 bytestim.plunkett
#84 drupal-1290694-84.patch87.11 KBtim.plunkett
#84 interdiff.txt7.13 KBtim.plunkett
#79 1290694_79.patch85.64 KBchx
#76 1290694_76.patch84.68 KBtlattimore
#74 1290694_74.patch84.6 KBtlattimore
#73 1290694_72.patch84.64 KBtlattimore
#71 1290694_71.patch84.2 KBtlattimore
#69 1290694_69.patch77.22 KBtlattimore
#68 1290694_68.patch84.73 KBchx
#68 interdiff.txt3.13 KBchx
#64 drupal_attributes-profile-top-excl.png194.91 KBmsonnabaum
#64 drupal_attributes-profile-overview.png43.1 KBmsonnabaum
#64 drupal_attributes-ab-diff.png75.76 KBmsonnabaum
#64 drupal-1290694-64.patch87.42 KBmsonnabaum
#62 drupal-1290694-62.patch87.44 KBc4rl
#61 drupal-1290694-61.patch87.48 KBc4rl
#58 drupal_array_attributes_1290694_58.patch84.72 KBhefox
#57 drupal_array_attributes_1290694_57.patch83.83 KBhefox
#56 drupal_array_attributes_1290694_56.patch83.83 KBhefox
#53 1290694_drupal_attri_54.patch83.41 KBhefox
#51 1290694_drupal_attri_52.patch83.85 KBhefox
#50 1290694_drupal_attri_50.patch83.54 KBhefox
#48 1290694_drupal_attri_48.patch83.35 KBhefox
#45 1290694_drupal_attri_45.patch83.34 KBhefox
#41 1290694_drupal_attri_41.patch82.96 KBhefox
#39 1290694_drupal_attri_39.patch81.59 KBhefox
#38 1290694_drupal_attri_39.patch81.89 KBhefox
#37 1290694_drupal_attri_36.patch82.95 KBhefox
#35 1290694_drupal_attri_35.patch82.95 KBhefox
#30 1290694_drupal_attri_30.patch80.22 KBhefox
#29 1290694_drupal_attri_28.patch80.28 KBhefox
#28 1290694_drupal_attri_27.patch77.87 KBhefox
#25 1290694_drupal_attri_25.patch77.81 KBhefox
#20 1290694_20.patch77.79 KBchx
#18 1290694_18.patch77.16 KBchx
#15 drupal-1290694-15.patch75.45 KBc4rl
#10 drupal-1290694-10.patch70.9 KBc4rl
#5 drupal-1290694-5.patch73.3 KBtim.plunkett
#4 attributes_array.patch73.29 KBhefox
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

subscribe

effulgentsia’s picture

At last weekend's San Francisco theme system sprint, chx wrote an Attributes class that implements ArrayAccess and __toString() that can allow us to simplify to a single $attributes variable that can automatically be an array when it needs to be, a string when it needs to be, and contain $attributes['class'] that can similarly handle array/string magic, and let the template choose whether to print class (or any other attribute) separate from other attributes or whether to just print $attributes containing everything. All in all, awesome stuff.

hefox started on preparing a patch that updates core to do all of the above. @hefox: any eta on when you can post a work-in-progress patch here?

effulgentsia’s picture

Component: markup » theme system
Issue tags: +consistency

Tagging and changing component to "theme system", since this doesn't affect the markup itself.

See also #1484704: Remove instances of attributes_array for a different, but related issue.

hefox’s picture

FileSize
73.29 KB

Here's a patch with the attributes class.

  • It changes attributes_array to always be attributes
  • It changes classes_array to be attributes[class].
  • drupal_attributes returns an instance of the new attribute class. With that, drupal_attributes can be used before/after/during making of the attribute information, whenver.
  • Calls drupal_attributes in template_preprocess_page when initializing the default variables (needed to be outside the default variable cache else all would be one object, ow), which thus means removes template_process (This does introduce some concern since it makes that class each call now, where before it only did anything when attributes array was not empty).
  • Updates core templates.

The one problem I know about is that

$attributes = new attribute();
$attributes['class'][] = 'class'

doesn't work (cause it doesn't know that class needs to be a attribute array class, etc.) and produces an ugly error. Looked briefly and looks like there may be a way around it based on something said on php.net, but wasn't able to figure it out.

works fine:

$attributes = new attribute();
$attributes['class'] = array();
$attributes['class'][] = 'class'

or

$attributes = new attribute(array('class' => array('class')));

I think I updated most places, but likely missed a spot. Been a week since patch was created.

tim.plunkett’s picture

FileSize
73.3 KB

Rerolled to still apply to core. Looks interesting for sure.

chx’s picture

Status: Active » Needs review
c4rl’s picture

Status: Needs review » Needs work

In template_preprocess_html():

    $variables['attributes']['class'][] = 'one-sidebar sidebar-first';

Shouldn't we adhere to arrays here?

    $variables['attributes']['class'][] = 'one-sidebar';
    $variables['attributes']['class'][] = 'sidebar-first';

That way we can safely use things like in_array() in preprocessors.

c4rl’s picture

Also in theme_form_element():

    $attributes['class'][] = 'form-item-' . strtr($element['#name'], array(' ' => '-', '_' => '-', '[' => '-', ']' => ''));

Seems like this should be

    $attributes['class'][] = drupal_html_class('form-item-' . $element['#name']);

I'm working on these cleanups in #7 and #8.

c4rl’s picture

Actually, nevermind #8, that's outside the scope of this issue. :)

c4rl’s picture

Status: Needs work » Needs review
FileSize
70.9 KB

Re-rolled with fixes from #7.

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

The last submitted patch, drupal-1290694-10.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#10: drupal-1290694-10.patch queued for re-testing.

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

The last submitted patch, drupal-1290694-10.patch, failed testing.

c4rl’s picture

Oops, forgot to include Drupal\Core\Template\Attribute ... re-rolling shortly.

c4rl’s picture

Status: Needs work » Needs review
FileSize
75.45 KB

There was an problem with Attribute::offsetSet as the $name parameter could be null. Let's try this one.

Also made Drupal\Core\Template\Attribute compatible with PSR-0.

Status: Needs review » Needs work

The last submitted patch, drupal-1290694-15.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
77.16 KB

Status: Needs review » Needs work

The last submitted patch, 1290694_18.patch, failed testing.

chx’s picture

FileSize
77.79 KB

One by one, the free templates of Drupal lands fell to the power of the Attributes...

chx’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1290694_20.patch, failed testing.

chx’s picture

I do not have more time tonight, whoever is going to work on this next please when you start working on it, use the Assigned field so we don't cross each other.

hefox’s picture

Assigned: Unassigned » hefox
hefox’s picture

Local test is fataling, so seeing what happens here.

hefox’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1290694_drupal_attri_25.patch, failed testing.

hefox’s picture

Status: Needs work » Needs review
FileSize
77.87 KB

Assuming I got the patch recreated correctly, this should fix the node title failure.

the node.tpl.php change needed to be

Edit: this should fix everything but the HTML Attributes test.

hefox’s picture

Can't get CommonDrupalAttributesUnitTestCase to run locally, atm (fatals), so here's a stab.

hefox’s picture

hm, the host of new failures is, uh, interesting.

hefox’s picture

Assigned: hefox » Unassigned
RobLoach’s picture

Status: Needs review » Needs work

Big fan of the clean up in this patch! Also really like the use of ArrayAccess. These are PHP tools that we definitely should take more advantage of. Some minor notes.....

+++ b/core/includes/common.incundefined
@@ -2250,11 +2250,7 @@ function drupal_http_header_attributes(array $attributes = array()) {
-    $data = $attribute . '="' . check_plain($data) . '"';
-  }
-  return $attributes ? ' ' . implode(' ', $attributes) : '';
+  return new Drupal\Core\Template\Attribute($attributes);
 }

We should probably have "use Drupal\Core\Template\Attribute;" at the top and "return new Attribute()" instead.

+++ b/core/includes/common.incundefined
@@ -5846,7 +5842,7 @@ function drupal_render(&$elements) {
   if (isset($elements['#pre_render'])) {
     foreach ($elements['#pre_render'] as $function) {
-      $elements = $function($elements);
+      $elements = call_user_func($function, $elements);
     }

Is there any reason for this change?

+++ b/core/includes/theme.incundefined
@@ -2480,22 +2462,24 @@ function template_preprocess_html(&$variables) {
   // This allows advanced theming based on context (home page, node of certain type, etc.).
   // Add a class that tells us whether we're on the front page or not.
-  $variables['classes_array'][] = $variables['is_front'] ? 'front' : 'not-front';
+  $variables['attributes']['class'][] = $variables['is_front'] ? 'front' : 'not-front';
   // Add a class that tells us whether the page is viewed by an authenticated user or not.
-  $variables['classes_array'][] = $variables['logged_in'] ? 'logged-in' : 'not-logged-in';
+  $variables['attributes']['class'][] = $variables['logged_in'] ? 'logged-in' : 'not-logged-in';

Absolutely love this clean up :-) .

+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -0,0 +1,66 @@
+namespace Drupal\Core\Template;
+use ArrayAccess;
+use Twig_Markup;

In some of the PSR-0 patches, I believe we had \ArrayAccess and \Twig_Markup instead of the use statements. I'm okay with either though :-) . It's just minor syntax. We could re-evaluate those later on globally.

+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -0,0 +1,66 @@
+class Attribute implements ArrayAccess {
+  protected $storage = array();
+  function __construct($attributes) {
+    foreach ($attributes as $name => $value) {
+      $this->offsetSet($name, $value);

Would be good to have a docblock for Attribute and the other classes.

Crell’s picture

In some of the PSR-0 patches, I believe we had \ArrayAccess and \Twig_Markup instead of the use statements. I'm okay with either though :-) . It's just minor syntax. We could re-evaluate those later on globally.

Um, they shouldn't have. Current coding standards say to "use" everything, even internal classes, and never have inline \. (Vis, what this patch is doing is correct.)

hefox’s picture

Is there any reason for this change?

chx from irc: RobLoach: as for $elements = call_user_func($function, $elements); it does not belong here but http://cdn.memegenerator.net/instances/400x/21190468.jpg

Current action items for this based on above two comments:

  • Add docblock for Attribute and the other classes.
  • "use Drupal\Core\Template\Attribute;" at the top and "return new Attribute()" instead.

I can update it tonight when I get home assuming I don't forget, but if I do forget or anyone sooner gets to it, go at it :).

hefox’s picture

Status: Needs work » Needs review
FileSize
82.95 KB

I'm not very good at writing comments :/

Status: Needs review » Needs work

The last submitted patch, 1290694_drupal_attri_35.patch, failed testing.

hefox’s picture

Status: Needs work » Needs review
FileSize
82.95 KB

Forgot to get rid of the drupal_render

hefox’s picture

hefox’s picture

Status: Needs review » Needs work

The last submitted patch, 1290694_drupal_attri_39.patch, failed testing.

hefox’s picture

hefox’s picture

Status: Needs work » Needs review
RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Looks great! Patch applied cleanly and everything is rendered exactly how it was before. Cleaner code, and we have more control over how it's outputted.

+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -0,0 +1,92 @@
+      return $this->storage[$name];
+    }
+  }
+
+  function offsetSet($name, $value) {
+    if (!is_object($value) || $value instanceOf Twig_Markup) {
+      if (is_array($value)) {
+        $value = new AttributeArray($name, $value);

Mind sticking notes in the issue summary to follow ups where we actually make use of Twig_Markup? Thanks!

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/theme.incundefined
@@ -2405,7 +2402,14 @@ function template_preprocess(&$variables, $hook) {
+  $variables += $default_variables + array(
+    'attributes' => drupal_attributes(array('class' => array())),
+    'title_attributes' => drupal_attributes(array('class' => array())),
+    'content_attributes' => drupal_attributes(array('class' => array())),
+  );

We're manually instantiating a new class here, three times for every call to template_process(). I'd like to know how expensive this is. Also it looks like we could potentially keep a static with the result of drupal_attributes(array('class' => array()); then copy that rather than building a new one from scratch?

+++ b/core/includes/theme.incundefined
@@ -2655,9 +2638,8 @@ function template_process_page(&$variables) {
+  // Flatten out html_attributes and attributes.
+  $variables['html_attributes'] = drupal_attributes($variables['html_attributes']);

This isn't really 'flatting out' is it any more?

+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -0,0 +1,92 @@
+ * To use, one may both pass in an array of already defined attributes and

array.syntax - space instead of a full stop?

+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -0,0 +1,92 @@
+ *  $attributes['class'][] = 'black-white-cat';

Does this actually work with ArrayAccess? I didn't think it could handle offsets yet.

i.e. you can do $foo = $bar['baz']; $foo['a'][] = 'b'; $bar['baz'] = $foo; but I don't know if just $foo['bar']['a'][] = 'b'; will work.

+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -0,0 +1,92 @@
+ * Indivudal parts of the attribute may be printed first.
+ * @code

Typo for 'individual'.

+++ b/core/lib/Drupal/Core/Template/AttributeArray.phpundefined
@@ -0,0 +1,58 @@
+namespace Drupal\Core\Template;
+use ArrayAccess;
+
+class AttributeArray extends AttributeValue implements ArrayAccess {

OK this answers my earlier question then, we're using ArrayAccess inside ArrayAccess to do this, nice :)

+++ b/core/lib/Drupal/Core/Template/AttributeString.phpundefined
@@ -0,0 +1,30 @@
+/**
+ * A class that the most standard HTML attribute.
+ *

This isn't a sentence...

+++ b/core/lib/Drupal/Core/Template/AttributeValue.phpundefined
@@ -0,0 +1,31 @@
+/**
+ * Defines the base class for an attribute type.
+ *
+ * @see Drupal\Core\Template\Attribute
+ */
+abstract class AttributeValue {
+  protected $printed = FALSE, $value, $name;
+
+  function __construct($name, $value) {
+    $this->name = $name;
+    $this->value = $value;
+  }
+
+  function render() {
+    return check_plain($this->name) . '="' . $this . '"';
+  }
+
+  function printed() {
+    return $this->printed;
+  }
+  abstract function __toString();

Base classes are supposed to be suffixed 'Base' now per coding standards.

+++ b/core/modules/block/block.tpl.phpundefined
@@ -10,10 +10,9 @@
+ * - $attributes: An object of HTML attributes that can be manipulated as an

Can we explicitly mention the Attributes class here? 'object of HTML attributes' is a bit non-committal.

+++ b/core/modules/field/field.moduleundefined
@@ -1069,13 +1069,12 @@ function template_process_field(&$variables, $hook) {
+  $variables['attributes'] = empty($variables['attributes']) ? '' : drupal_attributes($variables['attributes']);
+  $variables['title_attributes'] = empty($variables['title_attributes']) ? '' : drupal_attributes($variables['title_attributes']);

In template_preprocess() we were unconditionally assigning the attributes class, here we're keeping the ternary, why?

+++ b/core/modules/search/search-result.tpl.phpundefined
@@ -62,7 +60,7 @@
-<li class="<?php print $classes; ?>"<?php print $attributes; ?>>
+<li <?php print $attributes; ?>>

I love lines like this :)

hefox’s picture

array.syntax - space instead of a full stop?

The problem with showing invisible characters, sometimes periods look like spaces :P.

We're manually instantiating a new class here, three times for every call to template_process(). I'd like to know how expensive this is.
I'd like to know this also, but I don't know how to adequately test that (this is one of my main concerns with this patch).

Base classes are supposed to be suffixed 'Base' now per coding standards.
I was looking for example, but couldn't find it in the object oriented coding standards (wasn't sure if it'd be AttributeValueBase or AttributeBase, etc.), only the simpletest ones. Is this a pending coding standard? Changed to it though

Also it looks like we could potentially keep a static with the result of drupal_attributes(array('class' => array()); then copy that rather than building a new one from scratch?

Added a static to template_proprocess, but makes the question, should drupal_attributes have a static $default that it clones if the given array is blank?

Also, should drupal_attributes check and see if it's being called again (e.g. it's already Attributes object) and then just return the object? Seems safer but not sure the standards on that.

hefox’s picture

Status: Needs work » Needs review

Did I just accidently do a combo of blockquote and code above instead of just blockquote.. oops.

Status: Needs review » Needs work

The last submitted patch, 1290694_drupal_attri_45.patch, failed testing.

hefox’s picture

Status: Needs work » Needs review
FileSize
83.35 KB

Didn't rename AttributeValue.php

Status: Needs review » Needs work

The last submitted patch, 1290694_drupal_attri_48.patch, failed testing.

hefox’s picture

Status: Needs work » Needs review
FileSize
83.54 KB

hmm

hefox’s picture

Status: Needs review » Needs work

The last submitted patch, 1290694_drupal_attri_52.patch, failed testing.

hefox’s picture

Status: Needs work » Needs review
FileSize
83.41 KB

Reverted the use of clone.

Start with [class] in it already so as it's common enough to want class (as an array) in various places having to add the if (!isset(attributes class)) attributes class = array()) seemed undesirable.

That proves problematic for doing clone on a default drupal_attributes(array('class'=> array()) as the attributeArray 'class' object isn't cloned (since not a deep clone).

Could implement a copy constructor (assume php has one), not sure if it's warranted. Thoughts?

tim.plunkett’s picture

I think a good deal of the field-related fixes between #48 and #54 are actually due to #1625158: template_preprocess() doesn't run for theme_field()

Crell’s picture

I don't believe PHP has an explicit copy constructor. You'd use clone() instead, and the __clone() magic method to handle deep cloning.

Whether that's a good or bad performance trade-off to just creating a new object, I have no idea.

hefox’s picture

Should just be an update to latest 8.x of 53

hefox’s picture

This is with clone

Using a devel/php:

$time = microtime(1);
$memory = memory_get_usage();
for ($i = 0; $i < 1000; $i++) drupal_attributes(array('class' => array()));
drupal_set_message((microtime(1) - $time ) .' drupal_attributes ' . (memory_get_usage() - $memory));
$time = microtime(1);
$memory = memory_get_usage();
$default_attributes = drupal_attributes(array('class' => array()));
for ($i = 0; $i < 1000; $i++) clone $default_attributes;
drupal_set_message((microtime(1) - $time ) .' clone ' . (memory_get_usage() - $memory));

with output
0.013292074203491 drupal_attributes 64976
0.0047738552093506 clone 1680

So assuming I didn't mess that up, clone is significantly better.

hefox’s picture

bad patch

hefox’s picture

Alternate to clone in template_preprocess could do something like:

function drupal_attributes(array $attributes = array()) {
  static $default_attributes;
   if (empty($attributes)) {
    if (!isset($default_attributes)) {
      $default_attributes = new Attribute(array('class' => array()));
    }
    return clone $default_attributes;
  }
  elseif (is_object($attributes)) {
    return $attributes;
  }
  return new Attribute($attributes);
}
quicksketch’s picture

This patch looks pretty cool, though half-way implementations of "things that act like arrays but really are objects" can be frustrating to work with. This patch currently only implements ArrayAccess, but not Iterator. This means that you can't actually loop through all the available attributes, you can only check if known attributes exist or not. If we're going to be implementing ArrayAccess, we should also implement Iterator.

+ * Once it's turned into a string, it cannot be re-used.

This seems like an odd restriction. I would think that if you wanted to call print $someAttributeObject, then modify it, then print it again, it would reflect the changes.

+ * @see drupal_attributes()

In the documentation for Attributes, I'd love to see a more explicit PHPdoc that said, "Most of the time this object is not created directly, but instantiated by drupal_attributes()." The small @see drupal_attributes() understates the actual common usage.

+namespace Drupal\Core\Template;
+use ArrayAccess;
+use Twig_Markup;

There are a couple references to "Twig_Markup" classes. These are probably a bit premature to include in this patch.

c4rl’s picture

FileSize
87.48 KB

drupal_common_theme() was setting $variables['attributes'] to default to be an empty Array, and was bypassing class Attribute. I removed these, which also required checking isset($attributes) in Attribute::__construct to avoid PHP warnings.

c4rl’s picture

FileSize
87.44 KB

Bad patch. Sorry, that is_set was a silly change that was related to something else I was doing. :P

Status: Needs review » Needs work

The last submitted patch, drupal-1290694-62.patch, failed testing.

msonnabaum’s picture

I was seeing a significant regression with the latest patch (~10%), but we found an unnecessary check_plain() which accounted for most.

Here are the before and after with the attached patch, using the APC autoloader.


Also attached an ab diff, which slows a slightly larger regression, but I don't think the wall time diff is terribly meaningful in this case.

mlncn’s picture

*Everything* with this seems to work in my testing *except* the front page of Drupal when there are no nodes.

Somehow printing the title or calling drupal_set_message must be weird here-- the latter is called with PASS_THROUGH as a parameter?

It gives the error:

Warning: Invalid argument supplied for foreach() in Drupal\Core\Template\Attribute->__construct() (line 42 of core/lib/Drupal/Core/Template/Attribute.php).

And replaces the "Welcome to @site-name" with "Error"

Again, only on the front page without nodes is this a problem, everything else, and if any node is promoted, or a different front page set, it works.

chx’s picture

Status: Needs work » Needs review

Hrm, I think actually the one that msonnabaum uploaded is correct although it does revert the removal of array()s but I can not reproduce those on HEAD so I think it was a mistake during the other Twig work. Looking at theme_item_list, all the attributes are passed through drupal_attributes.

Status: Needs review » Needs work

The last submitted patch, drupal-1290694-64.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
84.73 KB

Oh that's because msonnabaum rerolled at the last min against c4rl's. This is what I meant.

tlattimore’s picture

FileSize
77.22 KB

This patch is a re-roll of chx's in #68 that removes the call to Twig_Markup, which quicksketch expressed concerns about in #60.

Status: Needs review » Needs work

The last submitted patch, 1290694_69.patch, failed testing.

tlattimore’s picture

Status: Needs work » Needs review
FileSize
84.2 KB

I forgot got add the core/lib/Drupal/Core/Template directory before diffing for the patch. Here is a re-roll of #69 which includes that, and will hopefully pass testing.

Status: Needs review » Needs work

The last submitted patch, 1290694_71.patch, failed testing.

tlattimore’s picture

Status: Needs work » Needs review
FileSize
84.64 KB

I mistakenly dropped all offsetSet() in Attribute.php do to a misunderstanding from chx in irc. This patch is simply a re-roll of #68.

tlattimore’s picture

FileSize
84.6 KB

This is exactly the same as #73, but also adds the documentation changes to Attribute.php that quicksketch requested in #60.

Status: Needs review » Needs work

The last submitted patch, 1290694_74.patch, failed testing.

tlattimore’s picture

Status: Needs work » Needs review
FileSize
84.68 KB

Missed a closing bracket in Attribute.php. Somehow removed it in the midst of futzing with re-rolling the patch.

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, tlattimore, and huge credit to hefox, tim.plunkett, c4l, msonnabaum, and chx. This is good to go in!

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

I really love (!!) this patch, but I'm afraid I have to mark it needs work once more.

+++ b/core/includes/common.inc
@@ -2270,11 +2271,7 @@ function drupal_http_header_attributes(array $attributes = array()) {
 function drupal_attributes(array $attributes = array()) {
...
+  return new Attribute($attributes);
 }

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -0,0 +1,101 @@
+ * Most of the time this object is not created directly, but
+ * instantiated by drupal_attributes().
+ *
+ * @see drupal_attributes()
+ */
+class Attribute implements ArrayAccess {

Any reason to keep drupal_attributes() around, since it is just a trivial wrapper? (There probably is a reason, I'm just asking.)

+++ b/core/lib/Drupal/Core/Template/Attribute.php
@@ -0,0 +1,101 @@
+  protected $storage = array();
+
+  function __construct($attributes = array()) {
...
+
+  function offsetGet($name) {

These, and all of the other functions and variables in the Attibrute* classes should be documented. I don't know what the standard is for stuff like __toString(), but I suspect we have a standard :)

+++ b/core/modules/system/html.tpl.php
@@ -47,7 +47,7 @@
-  <body class="<?php print $classes; ?>" <?php print $body_attributes;?>>
+  <body class="<?php print $attributes['class']; ?>" <?php print $attributes;?>>

+++ b/core/themes/seven/maintenance-page.tpl.php
@@ -22,7 +22,7 @@
-  <body class="<?php print $classes; ?>">
+  <body class="<?php print $attributes['class']; ?>">

Seems like this could print the attributes directly, no?

+++ b/core/modules/system/maintenance-page.tpl.php
@@ -21,7 +21,7 @@
-<body class="<?php print $classes; ?>">
+<body class="<?php print $attributes['class']; ?>">

Seems we could gain a feature at no cost, by simply printing attributes here?!

chx’s picture

Status: Needs work » Needs review
FileSize
85.64 KB

> Any reason to keep drupal_attributes() around, since it is just a trivial wrapper? (There probably is a reason, I'm just asking.)

Yes. That's a trivial followup. This is already a 80kb hard-to-read patch.

> These, and all of the other functions and variables in the Attibrute* classes should be documented

Hardly. Most method are just implementing ArrayAccess. In this patch I documented those that aren't.

> Seems like this could print the attributes directly, no?

Per jenlampton what template people most want is just to be able to add a class so while <body class="<?php print $attributes['class']; ?>" <?php print $attributes;?>> might look wasteful it's a TX boon because you can add a class without any PHP code.

I added iterators to our arrayaccess objects -- thanks to IteratorAggregate that was mighty easy. Edit: Funny when you Google IteratorAggregate the first non-php.net result is fabpot's blog post.

chx’s picture

Tagging.

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

And good to go again. @tstoeckler thank you for the review. Function __toString is as close to self-documenting as we can get, as are the other short methods within the Attributes class. Every method being documented is not a standard that i'm aware of and should not hold up this patch.

tstoeckler’s picture

Not marking "needs work" again, but from http://drupal.org/node/1354#functions:

All functions and methods, whether meant to be private or public, should be documented.

And then in http://drupal.org/node/1354#classes:

Each class and interface should have a doxygen documentation block, and each member variable, constant, and function/method within the class or interface should also have its own documentation block.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Reviewed & tested by the community » Needs work

Looking at http://api.drupal.org/api/search/8/offsetGet for example, @tstoeckler is right. It can't hurt, I'll do this now while I unwind from work. :)

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
7.13 KB
87.11 KB

I followed the lead of existing code in core, php.net docs, and http://drupal.org/node/1354#classes.
Re-RTBC, but there's an interdiff.

tstoeckler’s picture

Cool, awesome. RTBC++

neclimdul’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Template/Attribute.phpundefined
@@ -0,0 +1,144 @@
+  /**
+   * Implements the magic __construct() method.
+   */
+  public function __construct($attributes = array()) {

Sadness to complain about docs again but we really should document the argument since this our own implementation no an overridden method. I'd roll it myself but I'm not at my development machine.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
596 bytes
87.23 KB

Ran this by neclimdul in IRC, so marking back to RTBC.

tstoeckler’s picture

Sorry for not seeing this earlier, but for __construct() we do:

Constructs a Foo object.

(Where Foo is the class name.)

To not hold this up any longer attached patch with that change. Interdiff for easy review.
Leaving at RTBC, because it's such a small change, and - with the interdiff - should be verifiable by a committer. Hope that doesn't put anyone off.

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review

Actually, setting back to a quick "needs review". Again, should not really be hold-up.

tim.plunkett’s picture

Constructs a Drupal\Component\Plugin\Factory\DefaultFactory object.
Constructs a Delete object.
Initializes a new ArrayCollection.
Constructs a new cache backend.
(for Connection::__construct)
Implements Drupal\Core\Config\StorageInterface::__construct().

I fail to see a pattern. If we don't go with the patch in #87, I think we should use the full namespace for the class.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.13 KB
87.25 KB

Oh, maybe you're right.

Looking at our coding standards: http://drupal.org/node/1354#classes
The following is noted there:

  /**
   * Constructs a DatabaseStatementBase object.
   *
   * @param DatabaseConnection $dbh
   *   Database connection object.
   */
  protected function __construct($dbh) {
    // Function body here.
  }

That said the class there is not in any namespace, so...

I noted the need for a standard for class constructors in #1487760-45: [policy, no patch] Decide on documentation standards for namespaced items

Uploading a patch that uses the fully-qualified namespaces. Either this or #88 is RTBC, so marking as such. The comitter can decide which one is preferred. Since we already have an issue for defining and establishing a standard regarding namespaces (#1487760: [policy, no patch] Decide on documentation standards for namespaced items), working code should not be held up on that decision, IMO.

RobLoach’s picture

#91 is RTBC. The documentation update is minor, but does remove the meaningless doc block referencing itself. Let's stop arguing about minor doc syntax and get this in so it unblocks all the other issues that depend on this.

Crell’s picture

+++ b/core/includes/theme.inc
@@ -2449,23 +2427,22 @@ function template_preprocess_html(&$variables) {
+  $variables['html_attributes'] = drupal_attributes();

I know I'm late to the party here, but if drupal_attributes() is a one liner around new Attribute(), why bother with the function? It's just an extra stack call and a hard function dependency we don't need.

-24 days to next Drupal core point release.

RobLoach’s picture

I know I'm late to the party here, but if drupal_attributes() is a one liner around new Attribute(), why bother with the function? It's just an extra stack call and a hard function dependency we don't need.

Good call... Made a follow up for that one: #1700382: Replace remaining references to drupal_attributes() with new Attributes().

chx’s picture

This was raised in #78 and in #79 I did say it's a followup. Thanks for filing it tho so we wont forget.

Crell’s picture

Ah, cool. Sounds good to me.

effulgentsia’s picture

#91: drupal-1290694-91.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-1290694-91.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +consistency, +Drupal7AndTheArraysOfDoom

#91: drupal-1290694-91.patch queued for re-testing.

chx’s picture

Status: Needs review » Reviewed & tested by the community

The tests passed after a bot fluke so back to RTBC.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Nice clean-up that hopefully helps new designers and themers. Thanks!

aspilicious’s picture

Priority: Normal » Critical
Status: Fixed » Active
Issue tags: -consistency

This deserves a change notice. This affects several modules I know....

aspilicious’s picture

Title: Provide consistency for attributes and classes arrays provided by template_preprocess() » [change notice] Provide consistency for attributes and classes arrays provided by template_preprocess()
Issue tags: +consistency

damnit!

sun’s picture

Issue tags: +API change

Tagging.

tim.plunkett’s picture

Title: [change notice] Provide consistency for attributes and classes arrays provided by template_preprocess() » Change notification for: Provide consistency for attributes and classes arrays provided by template_preprocess()

Just for ... consistency!

hefox’s picture

I have no idea what I;m doing, but here's an attempt at a change notice: http://drupal.org/node/1727592

tstoeckler’s picture

I just removed the reference to drupal_attributes as that is on its way out. Otherwise looks good. I haven't quite grasped the nesting of Attributes yet myself, so cannot judge whether that is adequately documented. Hence, not setting RTBC.

tim.plunkett’s picture

It should mention drupal_attributes, but as the D7 way to do things. It is very helpful to have contrasting D7/D8 snippets to show how to update any D7 code using the old way.

chx’s picture

Status: Active » Fixed

I added a few words about the D7 drupal_attributes.

Tor Arne Thune’s picture

Title: Change notification for: Provide consistency for attributes and classes arrays provided by template_preprocess() » Provide consistency for attributes and classes arrays provided by template_preprocess()
Priority: Critical » Normal

Status: Fixed » Closed (fixed)

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

jenlampton’s picture

Issue tags: +Twig

adding tag

jenlampton’s picture

Issue summary: View changes

minor edit

podarok’s picture

Issue summary: View changes

Updated issue summary.

mitokens’s picture