Problem/Motivation
Adding classes via hook_preprocess_region() does not work (in Drupal 7).
Proposed resolution
- Add tests for Drupal 8 because it works there.
- Fix + add tests for Drupal 7.
Remaining tasks
Commit test patch to D8.
User interface changes
n/a
API changes
n/a
Original report by @jenlampton
It would be nice if adding classes into the correct part of a renderable would result in classes being added to the corresponding part of the HTML, wouldn't it? template_preprocess_region disagrees :/
Let's add something that checks the renderable and includes classes. This is a bit messy but it does the trick:
// Add classes to the region template, when needed.
if (!empty($variables['elements']['#attributes']['class'])){
$variables['classes_array'] = array_merge($variables['elements']['#attributes']['class'], $variables['classes_array']);
}
Comment | File | Size | Author |
---|---|---|---|
#55 | make_regions_classable-1530774-55.patch | 1001 bytes | dinarcon |
#49 | interdiff-1530774-49.txt | 668 bytes | lokapujya |
#49 | 1530774-49.patch | 1.98 KB | lokapujya |
#47 | interdiff.txt | 452 bytes | star-szr |
#47 | 1530774-47.patch | 1.99 KB | star-szr |
Comments
Comment #1
RobLoach#1290694: Provide consistency for attributes and classes arrays provided by template_preprocess() would help a lot.
Comment #2
jenlamptonYeah, but I still want a backport to D7, and the new attributes system is too complicated to get the whole thing backported.
I'd like to get a patch to get in to D8, and then *also* D7, before we replace the whole thing with better attributes. :)
Comment #3
jenlamptonimportant tagging
Comment #4
jenlamptonI think the approach in #2 should be preserved for D7, but we need to re-work this one for D8. Rerolling.
Comment #5
jenlamptonlet's remember to attach the patch this time.
Comment #5.0
jenlamptonclean up description a smidge
Comment #6
hpz CreditAttribution: hpz commentedJust rerolled the patch from #5, please review.
Comment #7
mathes CreditAttribution: mathes commentedpatch in #6 works for me.
Comment #8
headli CreditAttribution: headli commentedAs mathes mentioned, the patch works.
Comment #9
star-szrThis should probably have a test added, and has a minor coding standards issue:
Space between ) and { needed, see https://drupal.org/coding-standards#controlstruct.
Comment #10
lokapujyaComment #11
lokapujyaI'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
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">
Comment #12
star-szr@lokapujya thanks! Generally you would add classes via a preprocess function in your module/theme. Does that help?
Comment #13
lokapujyaThis works for manual testing:
Comment #14
lokapujyaWorking on a test. Ignore this one.
Comment #15
lokapujyaHere is an attempt at a test.
Comment #17
alexrayu CreditAttribution: alexrayu commentedPatch 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.
Comment #18
alexrayu CreditAttribution: alexrayu commentedBTW, Are you sure this code is manual testing for this patch? It succeeds even without applying the patch!
If this is how the patch needs to be tested, then the patch seems to be, in fact, unneeded.
Comment #19
star-szrWe can post a test-only version of the patch to see if the test fails, it should fail without the fix applied.
Comment #20
lokapujyaAgreed, 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?
Comment #21
lokapujyaTest Only patch.
Comment #22
star-szrYes, 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.
Comment #23
lokapujyaMoved the test to \Drupal\system\Tests\Theme\ThemeTest. Moved the test specific setup out of setUp() and into the test.
Comment #25
lokapujyarerolled.
Comment #27
lokapujyarerolled again.
Comment #28
lokapujyaComment #29
star-szrNice! Looking better, here's some more feedback:
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.
Is this used?
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?
The test shouldn't rely on Bartik and definitely shouldn't add preprocess callbacks on behalf of Bartik. Just do MODULENAME_preprocess_region().
I think we can come up with a better name for this test module than system_region_test, maybe theme_region_test?
This comment should probably follow our conventions and say:
Implements hook_preprocess_HOOK() for region templates.
Comment #30
lokapujyaImplemented suggestions from the last review.
Comment #31
svdhout CreditAttribution: svdhout commented#30 no longer applied, i rerolled the patch.
This is my very first patch, at the drupalcon Austin codesprint
Comment #32
star-szrThanks 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.
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?
What about just drupalGet('') here?
I don't think
hidden: true
is needed anymore, ref. #2209337: Remove "hidden" property from test extensionsRemove 'System module' from here please, I don't think it makes sense anymore.
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
Comment #33
lokapujyaI've made all the suggested changes.
Comment #34
lokapujyaBad Patch.
Comment #36
lokapujyaRemade patch against correct base with interdiff.
Comment #38
lokapujyaComment #39
star-szrThanks @lokapujya, I apologize for the delayed re-review.
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.
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.
Comment #40
lokapujyaThanks for reviewing. I implemented the suggestions.
Comment #41
lokapujyaneed to change the assertRaw()
Comment #43
star-szrThank 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 :)
Comment #44
lokapujyaChanged assertRaw() to assertTrue() of the cssSelect.
Comment #45
lokapujyaBetter cssSelect, removed debug.
Comment #47
star-szrOnly 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.
Comment #48
alexpottThe system module is already installed.
Comment #49
lokapujyaFixed.
Comment #50
star-szrThanks for the quick fix @lokapujya! And thanks @alexpott, welcome back :)
Comment #51
alexpottCommitted 2f17715 and pushed to 8.0.x. Thanks!
EDIT: didn't push had to commit again - thanks @cottser!
Comment #53
star-szrThanks @alexpott! Back to D7 for a test + fix there.
Comment #54
dinarcon CreditAttribution: dinarcon commentedWorking on this as part of core mentoring.
Comment #55
dinarcon CreditAttribution: dinarcon commentedModules' 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.
Comment #56
lokapujyaTried 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?
Comment #57
dinarcon CreditAttribution: dinarcon commented@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.
Comment #58
star-szrSee 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']
.Comment #59
star-szrAlso it's possible that originally this was talking about hook_page_alter(), it's hard to say. But maybe another place to check/test.
Comment #60
lokapujyaOk, I tried the manual test in #57 and the class now shows up with the patch applied. Now we need the automated test.
Comment #61
star-szr@lokapujya and the manual test fails without the patch, correct?
Comment #62
lokapujyaYes. 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.