Allow double underscores to pass through drupal_clean_css_identifier as per new CSS standards

+++ b/core/includes/common.incundefined
@@ -3359,7 +3359,7 @@ function drupal_delete_file_if_stale($uri) {
   $identifier = strtr($identifier, $filter);

from the php manual:

this function will be the most efficient when all the keys have the same size.

1. so there is an implied performance hit to this method,
2. also no actual 'translation' is being performed on '__' !!

because of these two negative factors, it does not feel like this is a 'fix', it's a workaround..
i think we at least need to consider is there a better alternative.

joates

I also removed the CSS tag (this concerns a php function and has no real connection to CSS i feel :)

the CSS coding standards also state that:

(identifiers) they cannot start with a digit, two hyphens, or a hyphen followed by a digit.

but drupal_clean_css_identifier does not currently test for that.

It seems to me that the best way forward is to re-factor this function to include all the necessary tests in an efficient way.

[edit]
this idea has already been proposed elsewhere and declined :/
(see comment #27 from #1109854: Overly aggressive transliteration in drupal_clean_css_identifier removes underscores from classes)
[/edit]

from comment #39 of #1109854: Overly aggressive transliteration in drupal_clean_css_identifier removes underscores from classes

My suggestion is that we should spin off the double underscore / double hyphen exception to a separate issue

gotta love the issue queue (NOT!) /** just a bit of sarcasm.. we have to make the best with the tools we have **/
i get to pretend i'm a CSI crime scene investigator for an hour before realizing i have completely wasted my time :/

@ry5n
sorry, my mistake, somebody else should give you a better review :))

Tested and works like a charm. Great work!

We should be adding more explicit tests for this function... to the method testDrupalCleanCSSIdentifier

Added test.

Just noticed that test passes also without applying the #1 patch. Guess I am doing something wrong...

You are right. Also the $identifier was wrong. It has a single underscore. Now it's fixed. Thanks @ry5n

Works for me, this is wonderful!

Looks great!

Committed 3360502 and pushed to 8.x. Thanks!

This should definitely be backported to 7.x, it's a pretty major blocker for themers moving into OOCSS using the new coding standards.

Isn't this an API change which could break people's Drupal 7 CSS?

I needed this for D7 so heres the D7 patch.

Isn't this an API change which could break people's Drupal 7 CSS?

Because of the way the patch is written it would simply allow double underscores to pass through. Which means you'd have to intentionally pass a double underscore through the function. AFAIK, no Drupal module does that on purpose expecting it to turn into a double dash.

The D7 patch in #16 worked for me. I agree with KrisBulman, this would definitely be nice to get in soon. Thanks for the patch!

Tested patch on D7 dev by passing a double underscore class name through drupal_html_class()

Also tested in Views, which uses drupal_clean_css_identifier() to pass class names through, it also works with this patch.

To clarify, I think the backwards compatibility problem here is with dynamically-generated class names. For example, the node module uses this function to generate classes based on each node type, so if there's a site that has a node type with a double underscore in the machine-readable name, this will change their HTML and likely break their theme if they have any styling specific to that node type.

Maybe we could do something like add a separate (wrapper) function which can be used by code that wants to adopt the new standard?

I also don't see this as major really? (Since nothing is actually broken, and there's already an easy workaround for code that wants to follow the new standard.)

Just to confirm that the patch from #16 worked for me. I had my underscores stripped on a class defined in the Views UI.

Emphasize that existing treatment of single underscores does not need to change.

so if there's a site that has a node type with a double underscore in the machine-readable name, this will change their HTML and likely break their theme if they have any styling specific to that node type.

That's 2 if's. Double underscore in the machine name seems really unlikely to me. The only double underscores that I know of are theme hook suggestions and they don't get passed to classes.

If you are really worried about that, we could alter the patch so that template_preprocess_node() did:

  $variables['classes_array'][] = drupal_html_class('node-' . strtr($node->type, array('__' => '--')));

That looks totally hacky to me, but its the only way to get Drupal 7 to adhere to the new Drupal CSS standards and to the hypothetical situation you propose.

Maybe we could do something like add a separate (wrapper) function which can be used by code that wants to adopt the new standard?

Nope. We can't. The current patch fixes the bug in drupal_clean_css_identifier() which is used by core's drupal_html_class(). And that function is used in core in several places: https://api.drupal.org/api/drupal/includes%21common.inc/function/calls/d... Everything passes through drupal_html_class(); there's no way to distinguish between UI-entered classes and auto-generated ones.

Since nothing is actually broken,

Yes, there is. Themers are getting CSS files from 3rd party libraries that have selectors like ".slide-show__navigation" and when they enter those class names into Views UI, Drupal changes the class name so that it no longer matches the selector in the CSS.

and there's already an easy workaround for code that wants to follow the new standard.

Really? What workaround? I don't see anything posted in the above comments about a workaround? The only work-around I see is forking the 3rd party libraries just to work with Drupal.

If Views is altering the classes that people paste into the Views UI any more than absolutely necessary, that's a bug in Views, not core. (And that includes single underscores just as much as double underscores.) It looks like the issue for fixing that is #1371118: Views row class filtering underscore to dashes on direct input and I just posted an updated patch there now. It's a simple fix, except for the backwards-compatibility issue (which the maintainers there were concerned about as well).

The same should be true for any other contrib module that allows UI-entered classes. I don't see where there's code in core that would be passing these UI-entered classes through drupal_html_class(); it would be the module itself that does that. There's already a relatively simple fix/workaround for any such module since the second parameter of drupal_clean_css_identifier() can be used to adjust the filtering, but what I was suggesting above was to add a helper function to core that makes that even simpler.

Handing over the responsibility to contrib module maintainers is not really a solution, as there is no documentation about this, neither for drupal_html_class(), what is mostly used, nor for drupal_clean_css_identifier().

There is only one line saying:
// By default, we filter using Drupal's coding standards.
But I didn't find any documented coding standards for css classes in Drupal 7. Is there one?

In fact, after stripping out the underscores, there is a documentation block that lists it as valid:

// Valid characters in a CSS identifier are:
  // - the hyphen (U+002D)
  // - a-z (U+0030 - U+0039)
  // - A-Z (U+0041 - U+005A)
  // - the underscore (U+005F)
  // - 0-9 (U+0061 - U+007A)
  // - ISO 10646 characters U+00A1 and higher
  // We strip out any character not in the above list.

What is right according to http://www.w3.org/TR/CSS21/syndata.html#characters.

But I think that for user entered html classes only non valid characters should be stripped, drupal_clean_css_identifier() is doing too much in that case anyway.

Apparently a lot of contrib modules are doing this wrong, it's not only a views issue. It is also present in title #2137295: Incorrect validation for custom wrapper classes and probably in many more modules.

tl;dr
As long as there are no coding standards for Drupal 7 that list underscores as non-allowed characters in html classes and considering the drawbacks pointed out here at least double-underscores should be allowed per default. If this is not possible a note about the right usage of these functions should be added in the documentation. And I guess having a new helper function focusing on class validation (not on converting a string into a class) would be great to have too.

> If Views is altering the classes that people paste into the Views UI any more than absolutely necessary, that's a bug in Views, not core.

Views isn't altering the class name. Core is. drupal_html_class() is a public API that is supposed to be used by contrib and core modules.

> Handing over the responsibility to contrib module maintainers is not really a solution

Agreed.

> There is only one line saying:
> // By default, we filter using Drupal's coding standards.

Yep. And this is a key. When we added drupal_html_class() its purpose was to massage strings into following Drupal's CSS standards, which were in a rough draft state at the time. Because we were passing drupal module names through it (like menu_block), we wanted to convert the single underscore into a single dash. No where was the "double underscore" convention used or discussed. They weren't disallowed or allowed by the CSS coding standards at the time.

The fact that double underscores (a BEM convention that is independent of Drupal) were converted to double dashes was an unintended consequence.

Since the code comment next to $filter says "By default, we filter using Drupal's coding standards" we should update the filter to match the new coding standards. And that's what this patch does.

The reason it's a bug in Views is that user input should not be forced to conform to Drupal's CSS standards. If the user enters 'some_class' via the Views UI it should not be converted to 'some-class' any more than 'some__class' should be converted to 'some--class'. In both cases Views should just use what they typed. So drupal_html_class() is the wrong function to call.

I get the problem that Drupal's CSS standards have changed and that drupal_html_class() no longer enforces them correctly, but changing what drupal_html_class() does in the middle of a stable release... I am just having trouble seeing how we can do that. It is very hard to predict what site themes it would actually break.

I think this issue could use more review from maintainers, so trying the new procedure from https://www.drupal.org/governance/core to add a tag for that...

Unfortunately we are using that in D7, it could potentially break layouts in many sites if it were be release in D7 now though very glad it made it into D8.

@David_Rothstein could we put in a variable_get('use_bem_friendly_drupal_clean_css_identifier', FALSE);(choose better name here) and make it configurable through $settings for people?

#30 sounds very reasonable.

A variable_get() as described in #30 sounds reasonable to me too. It's an extra function call, which is not great since this is a frequently-called function; however I guess it's not that frequently-called, so it seems OK.

So based on the last few comments, I guess the correct status here is "needs work"?

Here's a patch (for Drupal 7) that defaults to off, using the variable named preserve_css_double_underscores.

To enable, you can add this to your settings.php file:

  /**
   * Preserve CSS double underscores.
   */
  $conf['preserve_css_double_underscores'] = TRUE;

Here's another patch:

• Verifies existing Drupal 7 behavior is preserved.
• Tests that new behavior works by mocking a variable_set.
• Adds an example $conf in default.settings.php

1. +++ b/includes/common.inc
   @@ -3872,6 +3872,12 @@ function drupal_delete_file_if_stale($uri) {
   +  $preserve_css_double_underscores = variable_get('preserve_css_double_underscores', FALSE);
   +  if ($preserve_css_double_underscores) {
   +    $filter['__'] = '__';
   +  }
   

   I think it is worth to use drupal_static_fast pattern here to store the variable_get() call.

   In D8 we optimized the strtr away to save a lot of performance and here we add a function call. That feels wrong.

2. +++ b/modules/simpletest/tests/common.test
   @@ -888,6 +888,17 @@ class DrupalHTMLIdentifierTestCase extends DrupalUnitTestCase {
   +    // Mock a variable_set of preserve_css_double_underscores to TRUE.
   +    $GLOBALS['conf']['preserve_css_double_underscores'] = TRUE;
   

   Lets please clean this up again after the test as I am not sure that DrupalUnitTestCase does not clean this up itself.

Does this address your concerns? I just skipped the variable_get altogether.

@jtwalters, I think Fabianx told that it's better to use so-called drupal_static_fast pattern.

I'm sorry to interrupt. I adjusted some points from the patch #41.

1. Introduced drupal_static_fast pattern.
2. Fixed the error in the patch #41.
3. Adjusted the comment in default.settings.php.
4. Changed the variable name from preserve_css_double_underscores to allow_css_double_underscores.

There is already a variable allow_authorize_operations in Drupal and allow_xxx pattern is more popular as boolean variable name, I think. In respect of consistency and easiness, I think that allow_authorize_operations may be better.

But I'm not sure that allow_css_double_underscores is definitely better than preserve_css_double_underscores and any other names. I'd like someone to review this point. Thank you.

(edited)

Looks great to me, thanks for sticking to accessing variable_get(). In theory $conf should always be the same, but we generally try to avoid accessing global variables and use a proper API.

1. +++ b/includes/common.inc
   @@ -3893,6 +3893,21 @@ function drupal_delete_file_if_stale($uri) {
   +    $allow_css_double_underscores = variable_get('allow_css_double_underscores');
   

   variable_get('', FALSE);

2. +++ b/modules/simpletest/tests/common.test
   @@ -947,6 +947,21 @@ class DrupalHTMLIdentifierTestCase extends DrupalUnitTestCase {
   +    $this->assertIdentical(drupal_clean_css_identifier($identifier), $identifier, 'Verify double underscores are preserved.');
   

   I wonder how this passes.

   Something is definitely off here, because the static fast pattern would mean that this should not be set.

   Oh, I see, because of the missing, FALSE it returns NULL, so next time, isset() still returns FALSE.

   Okay, we need a drupal_static_reset('drupal_clean_css...:allow_...'); here (Insert proper name for ... obviously).

@Fabianx, thank you for your detailed review. I see. I fixed 2 points you pointed.

In addition, as you may think, to avoid affecting other test cases, another drupal_static_reset() call is added to the end of the test function (I created a helper function called resetDrupalCleanCSSIdentifierCache to DrupalHTMLIdentifierTestCase to just wrap drupal_static_reset()).

I tested this locally but I'd like it to be reviewed.

Thanks for your continued work on this. Here is some more feedback:

1. +++ b/modules/simpletest/tests/common.test
   @@ -956,12 +956,22 @@ class DrupalHTMLIdentifierTestCase extends DrupalUnitTestCase {
        $GLOBALS['conf']['allow_css_double_underscores'] = TRUE;
   +    $this->resetDrupalCleanCSSIdentifierCache();
   ...
        $GLOBALS['conf']['allow_css_double_underscores'] = $original;
   +    $this->resetDrupalCleanCSSIdentifierCache();
   

   I think if we go to the lengths of creating a helper function, which is a good thing I think, we should put the $GLOBALS['conf'] in their as well.

   And just call the helper something like:

   $this->setAllowCSSDoubleUnderscrores(TRUE);
   
   // Test
   
   $this->setAllowCSSDoubleUnderscrores(FALSE);
   

   I do not think we need to catch the original as else the test before us would have failed anyway, so we can assume the default.

2. +++ b/modules/simpletest/tests/common.test
   @@ -956,12 +956,22 @@ class DrupalHTMLIdentifierTestCase extends DrupalUnitTestCase {
   +   * reset the cache in drupal_clean_css_identifier().
   

   Needs some updates then, too.

@Fabianx, thank you for your assist. I revised the patch #46 following your feedback.

1. Introduced setAllowCSSDoubleUnderscrores() replacing resetDrupalCleanCSSIdentifierCache() for feedback 1.
2. Updated the doc comment on the setAllowCSSDoubleUnderscrores() for feedback 2.
3. And adjusted the comments for the above changes.

Here is a new patch and I'd like this to be reviewed again. Thank you.

+++ b/modules/simpletest/tests/common.test
@@ -947,6 +947,31 @@ class DrupalHTMLIdentifierTestCase extends DrupalUnitTestCase {
+  function setAllowCSSDoubleUnderscrores($value) {

nit - typo

@Fabianx, thank you and I'm sorry not to notice that. I fixed the typo #50: from setAllowCSSDoubleUnderscrores to setAllowCSSDoubleUnderscores. I tested it on my local environment.

edited: "rested it" to "tested it"

RTBC - Looks good to me now. Will leave to David or Stefan to commit though.

Overall this looks OK to me.

+++ b/sites/default/default.settings.php
@@ -616,3 +616,13 @@ $conf['404_fast_html'] = '<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN"
+ * CSS identifier double underscores allowance:
+ *
+ * To allow CSS identifiers to contain double underscores (.example__selector)
+ * for BEM-style naming standards, uncomment the line below.

Should we add a bit more explanation here, as well as a warning that turning on this variable for an existing site will change your CSS (or is that too obvious?)

That's great if we can make it Module Level only.

+ * for BEM-style naming standards, uncomment the line below.

Can we add "DRUPAL" here. `double underscores` (.block__element--modifier) is a more well-known standard that introduced by the csswizardry guy to the English-speaking world. By BEM authors, it has single underscore (.block__element_modifier)

#54: Could you clarify the exact wording you would like, please?

for BEM-style naming standards

to

for Drupal's BEM-style naming standards

Thanks for the reviews! I updated the patch #51.

1.

Should we add a bit more explanation here, as well as a warning that turning on this variable for an existing site will change your CSS (or is that too obvious?)

Surely it's unclear with the description. I added a note. Does this make it better?

 * Note that if you change this value in existing sites, existing page styles
 * may be broken.

2.

I added "Drupal's" before the word "BEM-style" following the droplet's thought.

Should we add the word "MindBEMding" (more common than "Drupal's BEM-style") to make it more clear?

3.

That's great if we can make it Module Level only.

@droplet, I'm sorry, I don't understand what "Module Level only" means. Could you please explain a little more?

Great to see some progress here!

Just a minor thing about the help text discussion: BEM Elements use double__underscores also in the original naming conventions: https://en.bem.info/methodology/naming-convention/#element-name So there is no need to mention a Drupal BEM style (that doesn't exist btw).

I would suggest the following:

To allow CSS identifiers to contain double underscores (e.g. to allow BEM-style naming standards like .block__element), uncomment the line below.

Back to RTBC

@criz,

check this part:
https://en.bem.info/methodology/naming-convention/#modifier-name

either way, we should clearly tell themer we don't allow single underscore, in the way to get Drupaler to accept csswizardry.com/2013/01/mindbemding-getting-your-head-round-bem-syntax/ (Or Drupal's code style guide) as only standard in Drupal

Basically, there're many variants of BEM. BEM has no strong solid guide & lack of examples. I can see everyone has their own points. Even in Drupal core, it has different style in some parts.

Re-reviewed with Fabianx, and committed and pushed to 7.x, thanks!

Great. Thank you!

Not meaning to reopen this issue, but trying to find out if there's any related issue to fix this in D8 - it's doing exactly the same thing there. Anyone know of an effort to apply this same kind of fix there?

@jpamental, if I understand correctly, double underscores are already preserved and we don't need to make a change in D8. See:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...

I am a front-end developer and I have encountered this issue with Drupal 8 and a View. 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".

I do not know what the underlying cause is, but the screenshots show where I enter the class names and then how they get output.

@artinruins
What the heck? I can confirm this bug and created an issue including a patch: #2849954: Double underscores are not preserved in main views CSS classes defined in views UI
Thanks for reporting it!