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.

CommentFileSizeAuthor
#49 interdiff-46-49.txt1.42 KBAnonymous (not verified)
#49 2849954-49.patch5.71 KBAnonymous (not verified)
#46 interdiff-41-45.txt2.5 KBAnonymous (not verified)
#46 2849954-css-classes-45.patch5.63 KBAnonymous (not verified)
#41 interdiff-35-41.txt1.17 KBacbramley
#41 2849954-css-classes-41.patch4.88 KBacbramley
#35 2849954-css-classes-35.patch4.67 KBMunavijayalakshmi
#30 2849954-css-classes-30.patch4.53 KBacbramley
#30 2849954-interdiff-19-30.txt2.25 KBacbramley
#19 interdiff-2849954-17-19.txt615 bytesshadcn
#19 double_underscores_are-2849954-19.patch4.22 KBshadcn
#17 2849954-17.patch4.24 KBacbramley
#17 interdiff.txt810 bytesacbramley
#14 2849954-css-classes-14.patch4.28 KBacbramley
#13 2849954-css-classes-13-test-only.patch3.46 KBacbramley
#9 allow-bem-in-views-css-classes-2849954-9.patch782 bytescriz
#7 allow-bem-in-views-css-classes-2849954-7.patch704 bytescriz
#5 2849954-views-css-class-5.patch611 bytesacbramley
#2 allow-bem-in-views-css-classes-2849954-2.patch603 bytescriz
drupal8-views-custom-class-OUTPUT.jpg55.43 KBcriz
drupal8-views-custom-class-INPUT.jpg100.18 KBcriz
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

criz created an issue. See original summary.

criz’s picture

Status: Active » Needs review
FileSize
603 bytes

Here is a patch.

dawehner’s picture

Issue tags: +Needs tests

Nice 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 :)

acbramley’s picture

Status: Needs review » Needs work

This doesn't work with multiple css classes in the same view, they are all concatenated with a dash instead.

acbramley’s picture

Here'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)!

Lendude’s picture

Version: 8.3.x-dev » 8.4.x-dev

I'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?).

criz’s picture

Status: Needs work » Needs review
FileSize
704 bytes

Your're right! Here is another patch that uses Html::cleanCssIdentifier(). I think we should use our own APIs when possible.

criz’s picture

Status: Needs review » Needs work
criz’s picture

Status: Needs work » Needs review
FileSize
782 bytes

Sorry, here a better version of the patch.

acbramley’s picture

+++ b/core/modules/views/views.theme.inc
@@ -34,8 +34,8 @@ function template_preprocess_views_view(&$variables) {
+    $variables['attributes']['class'] = array_map('\Drupal\Component\Utility\Html::cleanCssIdentifier', $css_classes);

Is 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!

criz’s picture

Status: Needs review » Needs work
acbramley’s picture

Assigned: Unassigned » acbramley

Working on tests for this

acbramley’s picture

Status: Needs work » Needs review
FileSize
3.46 KB

Here's a test-only patch to prove the failure.

acbramley’s picture

Assigned: acbramley » Unassigned
FileSize
4.28 KB

And the patch with array_merge.

The last submitted patch, 13: 2849954-css-classes-13-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: 2849954-css-classes-14.patch, failed testing.

acbramley’s picture

Status: Needs work » Needs review
FileSize
810 bytes
4.24 KB

Woops...going with a foreach instead.

Status: Needs review » Needs work

The last submitted patch, 17: 2849954-17.patch, failed testing.

shadcn’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
615 bytes

Hmm 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

acbramley’s picture

@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.

jazzper’s picture

Thanks 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).

shadcn’s picture

Status: Needs review » Reviewed & tested by the community

@jazzper thanks for the review. I believe it's RTBC then?

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs tests +Needs change record

Yay, 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.

acbramley’s picture

Status: Needs work » Needs review

I'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!

GiorgosK’s picture

any chance this will land in a 8.2.x release ?
at least 1 "view" that I take care of depends on it ;-)

shadcn’s picture

Status: Needs review » Reviewed & tested by the community

CR looking good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I'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?

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/views/views.theme.inc
@@ -34,8 +34,10 @@ function template_preprocess_views_view(&$variables) {
-    $variables['css_class'] = preg_replace('/[^a-zA-Z0-9- ]/', '-', $css_class);

Also this is removing a theme variable that potentially might be being used in custom templates.

acbramley’s picture

Assigned: Unassigned » acbramley
acbramley’s picture

Assigned: acbramley » Unassigned
Status: Needs work » Needs review
FileSize
2.25 KB
4.53 KB

Addressed 27 and 28 by adding back in the original logic as well for backwards compatibility.

jibran’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Thanks for adding the BC.

+++ b/core/modules/views/views.theme.inc
@@ -34,8 +34,14 @@ function template_preprocess_views_view(&$variables) {
+    $css_classes = array_map('\Drupal\Component\Utility\Html::cleanCssIdentifier', explode(' ', $css_class));

We are missing the strip_tags call from the last patch but I think that'd be fine.

acbramley’s picture

@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?

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/views.theme.inc
@@ -34,8 +34,14 @@ function template_preprocess_views_view(&$variables) {
     $variables['css_class'] = preg_replace('/[^a-zA-Z0-9- ]/', '-', $css_class);

I think we need to keep the expectation that this template variable contains all the sanitised classes. So this code should look something like:

  if (!empty($css_class)) {
    // Prior to Drupal 8.4, Views uses its own sanitisation method. This will
    // be preserved until Drupal 9.0 to keep backwards compatibility.
    $bc_classes = explode(' ', preg_replace('/[^a-zA-Z0-9- ]/', '-', $css_class));
    // Sanitize the classes using the classes using the proper API.
    $sanitized_classes = array_map('\Drupal\Component\Utility\Html::cleanCssIdentifier', explode(' ', $css_class));
    $variables['attributes']['class'] = array_unique(array_merge($bc_classes, $sanitized_classes));
    $variables['css_class'] = implode(' ', $variables['attributes']['class']);
  }

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.

Munavijayalakshmi’s picture

Assigned: Unassigned » Munavijayalakshmi
Munavijayalakshmi’s picture

Status: Needs work » Needs review
FileSize
4.67 KB
acbramley’s picture

+++ b/core/modules/views/views.theme.inc
@@ -34,8 +34,13 @@ function template_preprocess_views_view(&$variables) {
+    $variables['attributes']['class'] = array_unique(array_merge($bc_classes, $sanitized_classes));

Is it possible that classes could've been set before this preprocess hook?

Munavijayalakshmi’s picture

Assigned: Munavijayalakshmi » Unassigned
acbramley’s picture

@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!

alexpott’s picture

Status: Needs review » Needs work

I'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.

acbramley’s picture

Assigned: Unassigned » acbramley
acbramley’s picture

Assigned: acbramley » Unassigned
Status: Needs work » Needs review
FileSize
4.88 KB
1.17 KB

Updated with a check to the existing array, and merging if it exists.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ruloweb’s picture

Great patch!

+++ b/core/modules/views/views.theme.inc
@@ -34,8 +34,15 @@ function template_preprocess_views_view(&$variables) {
+    $bc_classes = explode(' ', preg_replace('/[^a-zA-Z0-9- ]/', '-', $css_class));

I think you missed the underscore here, '/[^a-zA-Z0-9- ]/' should be '/[^a-zA-Z0-9-_ ]/'.

acbramley’s picture

@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 :)

Anonymous’s picture

Great work here! Very useful!

Last unaddressed point: Add the test coverage to css_class variable, right? Done by #39.1.

hmartens’s picture

Awesome work! Will this make it into one of the releases soon like 8.6?

Lendude’s picture

@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.

  1. +++ b/core/modules/views/tests/src/Kernel/ViewsPreprocessTest.php
    @@ -0,0 +1,59 @@
    +class ViewsPreprocessTest extends ViewsKernelTestBase {
    +  /**
    

    missing a newline

  2. +++ b/core/modules/views/views.theme.inc
    @@ -34,8 +34,15 @@ function template_preprocess_views_view(&$variables) {
    +    // Prior to Drupal 8.5, Views uses its own sanitisation method. This will
    

    uses => used

  3. +++ b/core/modules/views/views.theme.inc
    @@ -34,8 +34,15 @@ function template_preprocess_views_view(&$variables) {
    +    // be preserved until Drupal 9.0 to keep backwards compatibility.
    +    $bc_classes = explode(' ', preg_replace('/[^a-zA-Z0-9- ]/', '-', $css_class));
    +    // Sanitize the classes using the classes using the proper API.
    +    $sanitized_classes = array_map('\Drupal\Component\Utility\Html::cleanCssIdentifier', explode(' ', $css_class));
    +    $view_classes = array_unique(array_merge($bc_classes, $sanitized_classes));
    

    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.

Anonymous’s picture

@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.

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

@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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/views.theme.inc
@@ -34,8 +34,17 @@ function template_preprocess_views_view(&$variables) {
+    $bc_classes = explode(' ', preg_replace('/[^a-zA-Z0-9- ]/', '-', $css_class));
+    // Sanitize the classes using the classes using the proper API.
+    $sanitized_classes = array_map('\Drupal\Component\Utility\Html::cleanCssIdentifier', explode(' ', $css_class));
+    $view_classes = array_unique(array_merge($bc_classes, $sanitized_classes));
+    // Merge the view display classes into any existing classes if they exist.
+    $variables['attributes']['class'] = !empty($variables['attributes']['class']) ? array_merge($variables['attributes']['class'], $view_classes) : $view_classes;
+    $variables['css_class'] = implode(' ', $view_classes);

Can 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:

$bc_classes = array_diff($bc_classes, $sanitized_classes)
if (!empty(array_diff($bc_classes, $sanitized_classes)) {
  @trigger_error('You might be using %s classes in your CSS. As of Drupal 9 they will not be supported. Use of one %s instead. See CR', E_USER_DEPRECATED);
 $sanitized_classes = array_unique(array_merge($bc_classes, $sanitized_classes));
}
$variables['attributes']['class'] = !empty($variables['attributes']['class']) ? array_merge($variables['attributes']['class'], $sanitized_classes) : $sanitized_classes;
$variables['css_class'] = implode(' ', $sanitized_classes);
Anonymous’s picture

Status: Needs work » Needs review

#51: We are only going to refuse the previous mechanism to sanitize $css_class value:

- preg_replace('/[^a-zA-Z0-9- ]/', '-', $css_class)
+ cleanCssIdentifier($css_class)

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:

if (\Drupal::config('views.settings')->get('use_previous_mechanism_sanitize_css_class')) {
 @trigger_error('You should not to rely on the previous mechanism to sanitize css classes. Disable it on the /admin/structure/views/settings). As of Drupal 9 it will not be supported. Use the necessary class value. See CR', E_USER_DEPRECATED);
 $sanitized_classes = array_unique(array_merge($bc_classes, $sanitized_classes));
}

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.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Yeah 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".

alexpott’s picture

Adding credit for reivews that affected the patch or helped managed the issue and BC layer.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 35d3174 and pushed to 8.6.x. Thanks!

diff --git a/core/modules/views/views.theme.inc b/core/modules/views/views.theme.inc
index 3f7e930f22..6028f3d450 100644
--- a/core/modules/views/views.theme.inc
+++ b/core/modules/views/views.theme.inc
@@ -34,10 +34,10 @@ function template_preprocess_views_view(&$variables) {
 
   $css_class = $view->display_handler->getOption('css_class');
   if (!empty($css_class)) {
-    // Views used its own sanitisation method. This will be preserved to keep
+    // Views uses its own sanitization method. This is preserved to keep
     // backwards compatibility.
-    // @todo: Decide what to do with this class, see
-    // https://www.drupal.org/project/drupal/issues/2977950
+    // @todo https://www.drupal.org/project/drupal/issues/2977950 Decide what to
+    //   do with the backwards compatibility layer.
     $bc_classes = explode(' ', preg_replace('/[^a-zA-Z0-9- ]/', '-', $css_class));
     // Sanitize the classes using the classes using the proper API.
     $sanitized_classes = array_map('\Drupal\Component\Utility\Html::cleanCssIdentifier', explode(' ', $css_class));

Fixed comment on commit.

  • alexpott committed 35d3174 on 8.6.x
    Issue #2849954 by acbramley, criz, vaplas, arshadcn, Munavijayalakshmi,...
Anonymous’s picture

Great thanks! 🎉

Anonymous’s picture

CR updated.

Status: Fixed » Closed (fixed)

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