Classes cannot be added into region.tpl.php via #attributes in preprocess

#1290694: Provide consistency for attributes and classes arrays provided by template_preprocess() would help a lot.

Just rerolled the patch from #5, please review.

patch in #6 works for me.

As mathes mentioned, the patch works.

This should probably have a test added, and has a minor coding standards issue:

+++ b/core/includes/theme.inc
@@ -2571,6 +2571,9 @@ function template_preprocess_region(&$variables) {
+  if (!empty($variables['elements']['#attributes']['class'])){

Space between ) and { needed, see https://drupal.org/coding-standards#controlstruct.

I'll get the reroll and the coding standards fix out of the way.

How do you add classes to the renderable? I did it by creating a custom module, then in hook_page_alter, I added

<?php
$page['content']['#attributes'] = array('class' => array('main', 'middle')); 
?>

With current D8 code, the classes already get added. When I apply this patch, the classes are doubled, see :
<div class="main middle main middle region region-content">

@lokapujya thanks! Generally you would add classes via a preprocess function in your module/theme. Does that help?

This works for manual testing:

<?php
function bartik_preprocess_region(&$variables) {
  $variables['attributes']['class'][] = 'new_class';
}
?>

Working on a test. Ignore this one.

Here is an attempt at a test.

Patch from 15 works for me. However, there is a small typo - a space is to be inserted after comma in arguments list. Line 37 of the patch. This is a simple style thing, so I am not offering an interdiff.

BTW, Are you sure this code is manual testing for this patch? It succeeds even without applying the patch!

<?php
function bartik_preprocess_region(&$variables) {
  $variables['attributes']['class'][] = 'new_class';
}
?>

If this is how the patch needs to be tested, then the patch seems to be, in fact, unneeded.

We can post a test-only version of the patch to see if the test fails, it should fail without the fix applied.

Agreed, and I even said that in #11. I think that is the correct manual test. Doh, I wrote a test for something that already works. It looks like this problem was already fixed by #1290694: Provide consistency for attributes and classes arrays provided by template_preprocess(). So, this can now be fixed in D7. Is there is any value in keeping the test in D8?

Test Only patch.

Yes, adding test coverage is almost always a good thing :D

As for feedback on the test, I think it should be added to \Drupal\system\Tests\Theme\ThemeTest because IMO it doesn't really fit in with the other tests in \Drupal\system\Tests\System\ThemeTest which are more about the 'Appearance' UI.

And the setup for this test (enabling modules, setting up content types, etc.) should be isolated to the test method not added to $modules and setUp(). Adding it to $modules and setUp() is effectively saying all test methods in that class need those in place and means each test in that class will take longer.

Moved the test to \Drupal\system\Tests\Theme\ThemeTest. Moved the test specific setup out of setUp() and into the test.

rerolled.

rerolled again.

Nice! Looking better, here's some more feedback:

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
   @@ -282,4 +282,23 @@ function testPreprocessHtml() {
   +   * Test that classes can be added into region.tpl.php via #attributes.
   

   Something along the lines of "Tests that region attributes can be manipulated via preprocess functions." would apply to both D7 and D8 and IMO be clearer about what this is testing.

2. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
   @@ -282,4 +282,23 @@ function testPreprocessHtml() {
   +    global $theme_key;
   

   Is this used?

3. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
   @@ -282,4 +282,23 @@ function testPreprocessHtml() {
   +    $this->drupalCreateContentType(array('type' => 'article', 'name' => 'Article'));
   +    $this->node = $this->drupalCreateNode(array('title' => t('Hello, world!'), 'type' => 'article'), NODE_PROMOTED);
   ...
   +    $this->drupalGet('node/1');
   

   Do we need to create a node and content type to test this or can we just place a block in a region like the "powered by" block instead?

4. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
   @@ -282,4 +282,23 @@ function testPreprocessHtml() {
   +    // Explicitly set the default and admin themes.
   +    $theme = 'bartik';
   +    theme_enable(array($theme));
   +    \Drupal::config('system.theme')->set('default', $theme)->save();
   
   +++ b/core/modules/system/tests/modules/system_region_test/system_region_test.module
   @@ -0,0 +1,13 @@
   +function bartik_preprocess_region(&$variables) {
   

   The test shouldn't rely on Bartik and definitely shouldn't add preprocess callbacks on behalf of Bartik. Just do MODULENAME_preprocess_region().

5. +++ b/core/modules/system/tests/modules/system_region_test/system_region_test.info.yml
   @@ -0,0 +1,7 @@
   +name: 'System region test'
   +type: module
   +description: 'Provides hook implementations for testing regions.'
   +package: Testing
   +version: VERSION
   +core: 8.x
   +hidden: true
   

   I think we can come up with a better name for this test module than system_region_test, maybe theme_region_test?

6. +++ b/core/modules/system/tests/modules/system_region_test/system_region_test.module
   @@ -0,0 +1,13 @@
   + * Add a classes can into region.tpl.php via #attributes.
   

   This comment should probably follow our conventions and say:

   Implements hook_preprocess_HOOK() for region templates.

Implemented suggestions from the last review.

#30 no longer applied, i rerolled the patch.
This is my very first patch, at the drupalcon Austin codesprint

Thanks for the updates @lokapujya and the reroll @svdhout! I think this is really close, but I have a few more questions and ideas about the test.

1. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
   @@ -282,4 +282,21 @@ function testPreprocessHtml() {
   +    // Explicitly set the default and admin themes.
   +    $theme = 'test_theme';
   +    theme_enable(array($theme));
   +    \Drupal::config('system.theme')->set('default', $theme)->save();
   ...
   +    \Drupal::service('router.builder')->rebuild();
   

   I am a bit curious if any of these lines are needed, and if they are needed I think a comment would be nice :) I don't think any theme needs to be enabled to test this and I'm not sure why the router would need to be rebuilt - the router one might play into my next comment perhaps?

2. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
   @@ -282,4 +282,21 @@ function testPreprocessHtml() {
   +    $this->drupalGet('node/1');
   

   What about just drupalGet('') here?

3. +++ b/core/modules/system/tests/modules/theme_region_test/theme_region_test.info.yml
   @@ -0,0 +1,7 @@
   +hidden: true
   

   I don't think hidden: true is needed anymore, ref. #2209337: Remove "hidden" property from test extensions

4. +++ b/core/modules/system/tests/modules/theme_region_test/theme_region_test.module
   @@ -0,0 +1,13 @@
   + * Provides System module hook implementations for testing purposes.
   

   Remove 'System module' from here please, I don't think it makes sense anymore.

5. +++ b/core/modules/system/tests/modules/theme_region_test/theme_region_test.module
   @@ -0,0 +1,13 @@
   +function theme_test_preprocess_region(&$variables) {
   

   This should match the naming convention of the module, so theme_region_test_preprocess_region(). When I said don't rely on Bartik I didn't mean switch to theme_test, modules can implement preprocess hooks too :D

I've made all the suggested changes.

Bad Patch.

Remade patch against correct base with interdiff.

Thanks @lokapujya, I apologize for the delayed re-review.

1. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
   @@ -282,4 +282,16 @@ function testPreprocessHtml() {
   +    $this->drupalPlaceBlock('system_powered_by_block');
   

   If we need to place this block maybe we can explain why. I'd think only one block placement would be needed for this test.

2. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
   @@ -282,4 +282,16 @@ function testPreprocessHtml() {
   +    $this->assertRaw('new_class', 'New class found.');
   

   Can we change this to use cssSelect instead? That would make the test more robust. https://www.drupal.org/node/2276689

   In addition to that maybe the preprocess could add a class to a specific region (sidebar_first in this case) rather than all regions… just a thought.

Thanks for reviewing. I implemented the suggestions.

need to change the assertRaw()

Thank you!

The cssSelect could check specifically that it's the sidebar_first region div (or whatever) that the class is being added to. So the CSS selector could be something more like .region-sidebar-first.new_class I think.

I cancelled testing on the interdiff.patch by the way, use .txt instead :)

Changed assertRaw() to assertTrue() of the cssSelect.

Better cssSelect, removed debug.

Only one nitpick left that I can see and that's a missing blank line between the last method and the ending of the class: https://www.drupal.org/node/608152#indenting

Rolled that one line change in here, RTBC from me and issue summary updated.

Thank you so much for sticking with this @lokapujya! Let's get this in and get a fix for D7 going. I can retitle after it's committed to 8.0.x but this is more accurate for what we're doing here.

+++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
@@ -278,4 +278,17 @@ function testPreprocessHtml() {
+    \Drupal::moduleHandler()->install(array('system', 'block', 'theme_region_test'));

The system module is already installed.

Fixed.

Thanks for the quick fix @lokapujya! And thanks @alexpott, welcome back :)

Committed 2f17715 and pushed to 8.0.x. Thanks!

EDIT: didn't push had to commit again - thanks @cottser!

Thanks @alexpott! Back to D7 for a test + fix there.

Working on this as part of core mentoring.

Modules' and themes' preprocess hooks are run after the default implementation template_preprocess_HOOK() so this functionality should be implemented in a process function. I have included a patch for this. A test would follow after some feedback.

Tried to manually test this. The patch applies great, but I tried the same test from #13 and did not see the classes show up. Is the test different for D7?

@lokapujya, following @jenlampton's suggestion in #2, we check for $variables['elements']['#attributes']['class'] so a manual test would be:

function HOOK_preprocess_region(&$variables) {
$variables['elements']['#attributes']['class'] = array('new-class');
}

This is for D7.

See the code sample in the issue summary, I think in D7 the class would not be added directly via $variables['attributes'] but by $variables['elements']['#attributes'].

Also it's possible that originally this was talking about hook_page_alter(), it's hard to say. But maybe another place to check/test.

Ok, I tried the manual test in #57 and the class now shows up with the patch applied. Now we need the automated test.

@lokapujya and the manual test fails without the patch, correct?

Yes. That was part of why I was having trouble manually testing it. When I tried reproducing the problem by adding the class to the classes_array, the class showed up even without the patch. But this specific way of adding the class did not work until I applied the patch. The automated test should confirm.