Problem/Motivation
Drupal’s CSS coding standards recommend a naming convention for classes which requires the use of double-underscores as separators in certain cases (BEM). However, when using the views UI to define a view class, underscores will be replaced with dashes. When defining a css class for a specific field, it works as expected.
This is a follow up issue from #2009584: Allow double underscores to pass through drupal_clean_css_identifier as per new CSS standards where artinruins reported the bug. See his screenshots attached. I can confirm this issue:
When adding a class via Edit View > Advanced > CSS, and classname that I add with a "__" double underscore will get rewritten on page render to "--" double hyphens. Drupal 8 is supposedly trying to make BEM (Block Element Modifier) CSS patterns the standard, but this messes with that standard. The accepted BEM pattern is "block__element--modifier".
Proposed resolution
Change the line
<?php
$variables['css_class'] = preg_replace('/[^a-zA-Z0-9- ]/', '-', $css_class)
?>
to allow underscores (e.g. by using '/[^a-zA-Z0-9-_ ]/'
for the regex).
Or better, use Html::cleanCssIdentifier().
CSS Changes
Be aware, that fixing this could break layout of existing sites. Specifically when underscores have been used in the main views css class setting.
Comment | File | Size | Author |
---|---|---|---|
#49 | interdiff-46-49.txt | 1.42 KB | Anonymous (not verified) |
#49 | 2849954-49.patch | 5.71 KB | Anonymous (not verified) |
#46 | interdiff-41-45.txt | 2.5 KB | Anonymous (not verified) |
#46 | 2849954-css-classes-45.patch | 5.63 KB | Anonymous (not verified) |
#41 | interdiff-35-41.txt | 1.17 KB | acbramley |
Comments
Comment #2
crizHere is a patch.
Comment #3
dawehnerNice and small fix. It is always better to be able to use our own APIs. I'm wondering whether it would be worth writing a test for this.
I guess my inner alexpott says yes :)
Comment #4
acbramley CreditAttribution: acbramley commentedThis doesn't work with multiple css classes in the same view, they are all concatenated with a dash instead.
Comment #5
acbramley CreditAttribution: acbramley commentedHere's a patch that implements the preg_replace change instead. The alternative would be to split the string on spaces and use Html::cleanCssIdentifier then concatenate again but that feels like a lot of work. Happy to write a test if someone can point me at place it should go, had a decent look through and it doesn't look like there's any tests around this stuff but I'm not too sure (it's a big test suite)!
Comment #6
LendudeI'm all for complying to CSS coding standards, but be warned, this will break existing sites, see #1262630: Raw value tokens not replaced if used in css class as an example.
I'd say this is a bad idea to do in a patch release, so bumping to 8.4. No idea what the standard is for CSS BC (is there such a thing?).
Comment #7
crizYour're right! Here is another patch that uses Html::cleanCssIdentifier(). I think we should use our own APIs when possible.
Comment #8
crizComment #9
crizSorry, here a better version of the patch.
Comment #10
acbramley CreditAttribution: acbramley commentedIs it possible for anything to add a class before this? Might need to do an array_merge or similar.
Otherwise looks good, +1 for using Drupal APIs!
Comment #11
crizComment #12
acbramley CreditAttribution: acbramley commentedWorking on tests for this
Comment #13
acbramley CreditAttribution: acbramley commentedHere's a test-only patch to prove the failure.
Comment #14
acbramley CreditAttribution: acbramley commentedAnd the patch with array_merge.
Comment #17
acbramley CreditAttribution: acbramley commentedWoops...going with a foreach instead.
Comment #19
shadcn CreditAttribution: shadcn at Chapter Three commentedHmm I believe single "_" would still be replaced. Updated the assert.
@acbramley thanks for looking into this. We found the same issue on a project. +1
Comment #20
acbramley CreditAttribution: acbramley commented@arshadcn thanks for that! I was about to look into that failing test as it seemed to be a deeper issue (i.e no markup was rendered at all) but must have been a testbot glitch.
Comment #21
jazzper CreditAttribution: jazzper commentedThanks for the patch. When I apply double_underscores_are-2849954-19.patch in the views module folder with:
git apply -v double_underscores_are-2849954-19.patch
Single "_" are replaced by "-". (double "__" not).
Comment #22
shadcn CreditAttribution: shadcn at Chapter Three commented@jazzper thanks for the review. I believe it's RTBC then?
Comment #23
xjmYay, it's great to see us actually using our APIs. I had thought that this was a design behavior, but I am taking @dawehner's comment above the signoff from the subsystem maintainer on the change, and of course since we already have a different API for this, that implies that we have a different design pattern addressing it already.
One question, I think this will actually change the classes that are output for affected Views displays? I think that is an okay change to make in a minor release, but probably we should have a small change record for it if so.
Comment #24
acbramley CreditAttribution: acbramley commentedI've drafted a CR here https://www.drupal.org/node/2855055
This is my first CR so please let me know if I'm missing detail!
Comment #25
GiorgosKany chance this will land in a 8.2.x release ?
at least 1 "view" that I take care of depends on it ;-)
Comment #26
shadcn CreditAttribution: shadcn at Chapter Three commentedCR looking good.
Comment #27
alexpottI've updated the CR a bit. However, given the fact that people have entered this in the UI for a reason of targeting them with their custom CSS are we should about making a BC breaking change here? Why not implement a BC layer where we do both the current behaviour and the new behaviour?
Comment #28
alexpottAlso this is removing a theme variable that potentially might be being used in custom templates.
Comment #29
acbramley CreditAttribution: acbramley commentedComment #30
acbramley CreditAttribution: acbramley commentedAddressed 27 and 28 by adding back in the original logic as well for backwards compatibility.
Comment #31
jibranThanks for adding the BC.
We are missing the strip_tags call from the last patch but I think that'd be fine.
Comment #32
acbramley CreditAttribution: acbramley commented@jibran yeah, I intentionally removed that as it didn't exist prior to this issue and I don't know if it's relevant for this particular problem?
Comment #33
alexpottI think we need to keep the expectation that this template variable contains all the sanitised classes. So this code should look something like:
I thought about adding an array_diff($bc_classes, $sanitized_classes) and doing a deprecation error (as per https://www.drupal.org/node/2856615) - but that's not right since proper usage could result in deprecation notices.
I'd be great to test the css_classes variable somehow.
Comment #34
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedComment #35
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedComment #36
acbramley CreditAttribution: acbramley commentedIs it possible that classes could've been set before this preprocess hook?
Comment #37
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedComment #38
acbramley CreditAttribution: acbramley commented@alexpott looks like the patch has been updated as per your code in #33, I'm not sure if my comment in #36 warrants a change?
Also agree with testing the variables, if someone knows how to get that raw variables array instead of matching the rendered markup I would love to know!
Comment #39
alexpottI've done some thinking about how to test the variable. As far as I can see we'd need a template in a test theme. Given that we are testing the classes added to
$variables['attributes']['class']
I think we should trust the implode is correct.@acbramley I think you are right to be concerned about classes that could have been set before - we should merge with the existing array here. To be consistent though
$variables['css_class']
should only contain the classes we are adding.Comment #40
acbramley CreditAttribution: acbramley commentedComment #41
acbramley CreditAttribution: acbramley commentedUpdated with a check to the existing array, and merging if it exists.
Comment #44
ruloweb CreditAttribution: ruloweb at Anexus, Phase2 commentedGreat patch!
I think you missed the underscore here, '/[^a-zA-Z0-9- ]/' should be '/[^a-zA-Z0-9-_ ]/'.
Comment #45
acbramley CreditAttribution: acbramley commented@ruloweb that line preserves exactly what the css_class key used to contain to maintain backwards compatibility for anyone using those classes in their themeing. We don't want to change it in any way :)
Comment #46
Anonymous (not verified) CreditAttribution: Anonymous commentedGreat work here! Very useful!
Last unaddressed point: Add the test coverage to
css_class
variable, right? Done by #39.1.Comment #47
hmartens CreditAttribution: hmartens commentedAwesome work! Will this make it into one of the releases soon like 8.6?
Comment #48
Lendude@hmartens@gmail.com It needs people to review it and then mark as RTBC if they feel its good enough.
Did a quick little review and found some nits and a @deprecated conundrum.
missing a newline
uses => used
Is there any way we can mark this as @deprecated? I can totally see this getting missed when going to D9. Should we do something along the lines of https://www.drupal.org/core/deprecation#how-unintended ? Split the BC logic into something we can trigger_error on? Not sure.
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commented@hmartens@gmail.com, thanks for up this issue!
@Lendude, thanks for review and hints!
#48.1: Done.
#48.2: Done.
#48.3: Yep. But since the deprecated is not the goal of this issue, maybe we can move this moment to the follow-up #2977950: Move the bc layer for views UI CSS classes from views to stable9.
Now, just provide the opportunity to set valid css classes via UI.
Comment #50
Lendude@vaplas Thanks for the clean up, and focussing on the fix here makes sense. Not 100% sold on doing some sort of deprecation notice in a follow up, but can't really come up with something better, so lets not delay this issue on that. Lets see what others think.
Comment #51
alexpottCan we trigger an deprecation error if $bc_classes is going to add something to the class list. That way we'll discover if core is depending on this and projects will be able to discover it too.
We can do something like:
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commented#51: We are only going to refuse the previous mechanism to sanitize
$css_class
value:But do not refuse the values:
'my--class'
->class="my--class" # OK
'my__class'
->class="my__class my--class" # bit oddly, but OK.
That is, if a user uses classes with two underscores, he should not receive a trigger error. Only if he clearly wants to preserve this previous mechanism. So we need something like:
Opened #2977950: Move the bc layer for views UI CSS classes from views to stable9 to explore this question. Or we should not postpone it to follow-up?
At the same time it's really interesting to check the core by code from #51. Done in #2948180-44. Only current test fails. So, in the core all good.
Comment #53
alexpottYeah this is one of those tricky situations. Because you can never know if a site is using it. I think it discussing how best to do this in a separate issue is a good idea. It's good to know that core doesn't use this "feature".
Comment #54
alexpottAdding credit for reivews that affected the patch or helped managed the issue and BC layer.
Comment #55
alexpottCommitted 35d3174 and pushed to 8.6.x. Thanks!
Fixed comment on commit.
Comment #57
Anonymous (not verified) CreditAttribution: Anonymous commentedGreat thanks! 🎉
Comment #58
Anonymous (not verified) CreditAttribution: Anonymous commentedCR updated.