Problem/Motivation

Originally this issue was a question on standardizing on 'attributes_array' vs '#attributes' as attributes array keys in render arrays and preprocessors. Despite resolution in consistency via #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess(), there are still some instances of attributes_array. For consistency sake, we should get rid of these

Proposed resolution

Convert/remove instances of attributes_array. Grep can find these.

Remaining tasks

Patch

User interface changes

None

API changes

None (That is, none not already accommodated by other issues).

Original report by effulgentsia

#1382350-18: [discussion] Theme/render system problems laid out some WTFs of our render/theme systems, and other comments on that issue echoed the desire for more consistency. This seems like an obvious one to fix.

Files: 
CommentFileSizeAuthor
#21 interdiff.txt2.4 KBEric_A
#21 1484704-21-replace-attributes_array.patch2.67 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 55,894 pass(es).
[ View ]
#16 1484704-16-replace-attributes_array.patch4.83 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1484704-16-replace-attributes_array.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 1484704-12-replace-attributes_array.patch5.94 KBjoelpittet
PASSED: [[SimpleTest]]: [MySQL] 55,637 pass(es).
[ View ]
#12 interdiff.txt1.09 KBjoelpittet
#10 1484704-10-replace-attributes_array.patch5.94 KBjoelpittet
FAILED: [[SimpleTest]]: [MySQL] 55,360 pass(es), 0 fail(s), and 86 exception(s).
[ View ]
#7 drupal_core_8x-replace_instances_of_attributes_array-148704-7.patch2.96 KBelvis2
PASSED: [[SimpleTest]]: [MySQL] 48,775 pass(es).
[ View ]
#5 1484704-attributes-array-removal-5.patch3.09 KBwiifm
FAILED: [[SimpleTest]]: [MySQL] 48,198 pass(es), 0 fail(s), and 133 exception(s).
[ View ]

Comments

pixelwhip’s picture

+1 for $element['#attributes']

effulgentsia’s picture

c4rl’s picture

Issue tags:-Novice

Now that #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess() is committed, I am thinking that this is now a task to simply get rid of attributes_array instances since the new Attributes class applies.

A grep reveals the following:

./core/includes/theme.inc: *   modify the $variables['attributes_array'] variable, and after all of them

./core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.php:    $this->assertEqual($variables2['content_attributes']['class'], array('test-class', 'content'), 'Default .content class added to block content_attributes_array');

./core/modules/system/lib/Drupal/system/Tests/Common/AttributesUnitTest.php:    $attributes_array = array('key1' => 'value1');

./core/modules/system/lib/Drupal/system/Tests/Common/AttributesUnitTest.php:    $attribute = new Attribute($attributes_array);

./core/modules/views/theme/theme.inc:    $vars['attributes_array'] = array('summary' => $handler->options['summary']);

./core/modules/views/theme/theme.inc:    $vars['attributes_array'] = array('summary' => $handler->options['summary']);

./core/modules/views/views_ui/theme/theme.inc:    $variables['attributes_array']['title'][] = t('Overridden');

./core/modules/views/views_ui/theme/theme.inc:    $variables['attributes_array']['title'][] = t('Overridden');

c4rl’s picture

Title:Standardize on either $variables['attributes_array'] or $element['#attributes'], and remove the other one» Remove instances of attributes_array
Issue tags:+Novice

Adding novice tag, this should be relatively straightforward.

wiifm’s picture

Status:Active» Needs review
Issue tags:+Novice
StatusFileSize
new3.09 KB
FAILED: [[SimpleTest]]: [MySQL] 48,198 pass(es), 0 fail(s), and 133 exception(s).
[ View ]

Okay here is my crack at this. Firstly a grep of the latest 8.x code:

seanh@wiifmonthego /var/www/D8 git:8.x » grep -nriI "attributes_array" *
core/includes/theme.inc:848: *   modify the $variables['attributes_array'] variable, and after all of them
core/modules/views/theme/theme.inc:661:    $vars['attributes_array'] = array('summary' => $handler->options['summary']);
core/modules/views/theme/theme.inc:783:    $vars['attributes_array'] = array('summary' => $handler->options['summary']);
core/modules/views/views_ui/theme/theme.inc:32:    $variables['attributes_array']['title'][] = t('Overridden');
core/modules/views/views_ui/theme/theme.inc:49:    $variables['attributes_array']['title'][] = t('Overridden');
core/modules/system/lib/Drupal/system/Tests/Common/AttributesUnitTest.php:51:    $attributes_array = array('key1' => 'value1');
core/modules/system/lib/Drupal/system/Tests/Common/AttributesUnitTest.php:52:    $attribute = new Attribute($attributes_array);
core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.php:54:    $this->assertEqual($variables2['content_attributes']['class'], array('test-class', 'content'), 'Default .content class added to block content_attributes_array');

And after patching:

seanh@wiifmonthego /var/www/D8 git:1484704-attributes-array » grep -nriI "attributes_array" *
core/modules/system/lib/Drupal/system/Tests/Common/AttributesUnitTest.php:51:    $attributes_array = array('key1' => 'value1');
core/modules/system/lib/Drupal/system/Tests/Common/AttributesUnitTest.php:52:    $attribute = new Attribute($attributes_array);
core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.php:54:    $this->assertEqual($variables2['content_attributes']['class'], array('test-class', 'content'), 'Default .content class added to block content_attributes_array');

As you can see, some instances of the word 'attributes_array' remain in the codebase, the first two seem correct to me, as it is simply the name of a temporary variable in tests. The last instance also looks like a harmless UI string, and is in tests.

Lets see what the test bot says

Status:Needs review» Needs work

The last submitted patch, 1484704-attributes-array-removal-5.patch, failed testing.

elvis2’s picture

Status:Needs work» Needs review
StatusFileSize
new2.96 KB
PASSED: [[SimpleTest]]: [MySQL] 48,775 pass(es).
[ View ]

Variable names were not changed (ie $attributes_array remained the same).

joelpittet’s picture

Status:Needs review» Needs work
Issue tags:+Quick fix

@elvis2 #7 looks good but should be ['attributes'] not ['#attributes']

These are the ones I found:
attributes_array.jpg

The system ones are taken care of at #1183042-93: Regression: Add WAI-ARIA roles to Core blocks

joelpittet’s picture

Status:Needs work» Needs review
StatusFileSize
new5.94 KB
FAILED: [[SimpleTest]]: [MySQL] 55,360 pass(es), 0 fail(s), and 86 exception(s).
[ View ]

Tried to apply #7 but it didn't apply anymore, so here is a re-roll.

Status:Needs review» Needs work

The last submitted patch, 1484704-10-replace-attributes_array.patch, failed testing.

joelpittet’s picture

Status:Needs work» Needs review
StatusFileSize
new1.09 KB
new5.94 KB
PASSED: [[SimpleTest]]: [MySQL] 55,637 pass(es).
[ View ]

Fixed the title attribute to not set an array.

elvis2’s picture

@joelpittet Thanks for pointing the error out. I will fix it later this evening.

joelpittet’s picture

Issue tags:-Novice

#12 should be good to go. Could someone review/RTBC?

Eric_A’s picture

Status:Needs review» Needs work

I'm guessing the WAI-ARIA roles got into #10 and #12 by accident. Those are still being discussed in #1183042: Regression: Add WAI-ARIA roles to Core blocks where @alexpott and @webchick suggested adding a test.

@dawehner RTBC'ed the patch from #41 in #1843750: Convert views/templates/views-view-grid.tpl.php to twig so perhaps he'll do the same here when WAI-ARIA roles are out. :-)

Eric_A’s picture

Status:Needs work» Needs review
StatusFileSize
new4.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1484704-16-replace-attributes_array.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is #12 with the WAI-ARIA roles changes reverted.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

I'm no @dawehner, but I feel comfortable RTBC'ing this.

One little thing stuck out at me:

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/AttributesUnitTest.php
@@ -48,8 +48,8 @@ function testDrupalAttributes() {
+    $attributes = array('key1' => 'value1');
+    $attribute = new Attribute($attributes);

For extra credit, this could be put into one-line.

I'll leave it to the committers (or whoever else) to decide, whether they deem that worthy of another re-roll.

alexpott’s picture

Issue tags:-Quick fix, -consistency

Status:Reviewed & tested by the community» Needs work
Issue tags:+Quick fix, +consistency

The last submitted patch, 1484704-16-replace-attributes_array.patch, failed testing.

joelpittet’s picture

Issue tags:+Needs reroll
Eric_A’s picture

Status:Needs work» Needs review
StatusFileSize
new2.67 KB
PASSED: [[SimpleTest]]: [MySQL] 55,894 pass(es).
[ View ]
new2.4 KB

So a couple of PHPTemplate to Twig conversion issues considered it their responsibility to partially fix this issue ;-)

Status:Needs review» Needs work

The last submitted patch, 1484704-21-replace-attributes_array.patch, failed testing.

Eric_A’s picture

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

And removed the "Needs reroll" tag.

Eric_A’s picture

EDIT: my patch applied just fine and my checks were done after applying it..
Ok, I just checked core/includes/theme.inc and it too has no instances of $attributes_array anymore. Me, I'm not going to check right now if every line was fixed in the exact same way as the patch here, but basically we're done here, I guess.

EDIT: I also checked core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.php. Same thing. Fixed by some other issue.

Eric_A’s picture

Hm, then why did my patch in #21 apply locally...

(Yeah, just a testbot glitch here: FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].)

Eric_A’s picture

Eric_A’s picture

Status:Needs review» Reviewed & tested by the community

We're back to green and I'm setting this back to RTBC (see #17). To summarize: patch from #16 did not apply anymore because the 4 lines from views.theme.inc and views_ui.theme.inc were fixed in #1843772: Convert views/views_ui/templates/views-ui-display-tab-bucket.tpl.php to Twig, #1843774: Convert views/views_ui/templates/views-ui-display-tab-setting.tpl.php to Twig, #1843750: Convert views/templates/views-view-grid.tpl.php to twig and #1843766: Convert views/templates/views-view-table.tpl.php to twig. I reverted those changes here and the patch applied again. Then I added the bonus request from #17. The interdiff shows how trivial the changes are.

catch’s picture

Status:Reviewed & tested by the community» Fixed

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

Anonymous’s picture

Issue summary:View changes

Issue summary template