Problem/Motivation
Drupal’s new CSS coding standards recommend a naming convention for classes which requires the use of double-underscores as separators in certain cases. However, Drupal currently processes some classes internally using drupal_clean_css_identifier(), which replaces all underscores with dashes.
Proposed resolution
Allow double underscores to pass through drupal_clean_css_identifier untouched, if the allow_css_double_underscores variable is enabled. Because many modules rely on the current behavior of this function in order to generate standard dash-separated class names from php strings such as field_name
(and this does not need to change), we should for the time being retain the existing behaviour for single underscores. #1109854: Overly aggressive transliteration in drupal_clean_css_identifier removes underscores from classes exists to discuss altering the treatment of single underscores.
Remaining tasks
None.
User interface changes
None.
API changes
None.
Related Issues
#1995272: [Meta] Refactor module CSS files inline with our CSS standards
#1109854: Overly aggressive transliteration in drupal_clean_css_identifier removes underscores from classes
Comment | File | Size | Author |
---|---|---|---|
#70 | drupal8-views-custom-class-OUTPUT.jpg | 55.43 KB | artinruins |
#70 | drupal8-views-custom-class-INPUT.jpg | 100.18 KB | artinruins |
#57 | interdiff-2009584-51-56.txt | 596 bytes | hgoto |
#57 | allow_double-2009584-56.patch | 3.73 KB | hgoto |
#16 | drupal-remove_double_underscore_from_css_filter-2009584-16.patch | 632 bytes | emattias |
Comments
Comment #1
ry5n CreditAttribution: ry5n commentedI’m attaching the last patch from #1109854: Overly aggressive transliteration in drupal_clean_css_identifier removes underscores from classes, posted in #33 by @mbrett5062.
I’ve tested this function separately and it seems to work as needed. Can I get a seconder?
Comment #1.0
ry5n CreditAttribution: ry5n commentedClarify OP and link to the CSS standards.
Comment #1.1
ry5n CreditAttribution: ry5n commentedUpdated issue summary.
Comment #2
joates CreditAttribution: joates commentedfrom the php manual:
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 :)
Comment #3
joates CreditAttribution: joates commentedthe CSS coding standards also state that:
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]
Comment #4
joates CreditAttribution: joates commentedfrom comment #39 of #1109854: Overly aggressive transliteration in drupal_clean_css_identifier removes underscores from classes
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 :))
Comment #5
rteijeiro CreditAttribution: rteijeiro commentedTested and works like a charm. Great work!
Comment #6
alexpottWe should be adding more explicit tests for this function... to the method
testDrupalCleanCSSIdentifier
Comment #7
rteijeiro CreditAttribution: rteijeiro commentedAdded test.
Comment #8
rteijeiro CreditAttribution: rteijeiro commentedJust noticed that test passes also without applying the #1 patch. Guess I am doing something wrong...
Comment #9
ry5n CreditAttribution: ry5n commented@rteijeiro
You’re passing an empty array as the filter, which overrides the default in the function. I suspect this would work:
Comment #10
rteijeiro CreditAttribution: rteijeiro commentedYou are right. Also the $identifier was wrong. It has a single underscore. Now it's fixed. Thanks @ry5n
Comment #11
carwin CreditAttribution: carwin commentedWorks for me, this is wonderful!
Comment #12
KrisBulman CreditAttribution: KrisBulman commentedLooks great!
Comment #13
alexpottCommitted 3360502 and pushed to 8.x. Thanks!
Comment #14
KrisBulman CreditAttribution: KrisBulman commentedThis should definitely be backported to 7.x, it's a pretty major blocker for themers moving into OOCSS using the new coding standards.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedIsn't this an API change which could break people's Drupal 7 CSS?
Comment #16
emattias CreditAttribution: emattias commentedI needed this for D7 so heres the D7 patch.
Comment #17
JohnAlbinBecause 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.
Comment #18
guschilds CreditAttribution: guschilds commentedThe D7 patch in #16 worked for me. I agree with KrisBulman, this would definitely be nice to get in soon. Thanks for the patch!
Comment #19
KrisBulman CreditAttribution: KrisBulman commentedComment #20
KrisBulman CreditAttribution: KrisBulman commentedTested 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.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedTo 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?
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.)
Comment #23
drupov CreditAttribution: drupov commentedJust to confirm that the patch from #16 worked for me. I had my underscores stripped on a class defined in the Views UI.
Comment #23.0
drupov CreditAttribution: drupov commentedEmphasize that existing treatment of single underscores does not need to change.
Comment #25
JohnAlbinThat'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: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.
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.
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.
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.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedIf 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.
Comment #27
crizHanding 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 fordrupal_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:
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.
Comment #28
JohnAlbin> 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.
Comment #29
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThe 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...
Comment #30
joelpittetUnfortunately 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?Comment #31
drupov CreditAttribution: drupov commented#30 sounds very reasonable.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedA 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.
Comment #33
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSo based on the last few comments, I guess the correct status here is "needs work"?
Comment #34
jtwalters CreditAttribution: jtwalters commentedHere'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:
Comment #35
jtwalters CreditAttribution: jtwalters commentedComment #37
jtwalters CreditAttribution: jtwalters commentedHere's another patch:
Comment #38
jtwalters CreditAttribution: jtwalters commentedComment #40
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedI 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.
Lets please clean this up again after the test as I am not sure that DrupalUnitTestCase does not clean this up itself.
Comment #41
jtwalters CreditAttribution: jtwalters commentedDoes this address your concerns? I just skipped the variable_get altogether.
Comment #42
jtwalters CreditAttribution: jtwalters commentedComment #44
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commented@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.
preserve_css_double_underscores
toallow_css_double_underscores
.There is already a variable
allow_authorize_operations
in Drupal andallow_xxx
pattern is more popular as boolean variable name, I think. In respect of consistency and easiness, I think thatallow_authorize_operations
may be better.But I'm not sure that
allow_css_double_underscores
is definitely better thanpreserve_css_double_underscores
and any other names. I'd like someone to review this point. Thank you.(edited)
Comment #45
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedLooks 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.
variable_get('', FALSE);
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).
Comment #46
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commented@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 calledresetDrupalCleanCSSIdentifierCache
toDrupalHTMLIdentifierTestCase
to just wrapdrupal_static_reset()
).I tested this locally but I'd like it to be reviewed.
Comment #47
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commentedComment #48
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThanks for your continued work on this. Here is some more feedback:
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:
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.
Needs some updates then, too.
Comment #49
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commented@Fabianx, thank you for your assist. I revised the patch #46 following your feedback.
setAllowCSSDoubleUnderscrores()
replacingresetDrupalCleanCSSIdentifierCache()
for feedback 1.setAllowCSSDoubleUnderscrores()
for feedback 2.Here is a new patch and I'd like this to be reviewed again. Thank you.
Comment #50
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentednit - typo
Comment #51
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commented@Fabianx, thank you and I'm sorry not to notice that. I fixed the typo #50: from
setAllowCSSDoubleUnderscrores
tosetAllowCSSDoubleUnderscores
. I tested it on my local environment.edited: "rested it" to "tested it"
Comment #52
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - Looks good to me now. Will leave to David or Stefan to commit though.
Comment #53
stefan.r CreditAttribution: stefan.r commentedOverall this looks OK to me.
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?)
Comment #54
droplet CreditAttribution: droplet commentedThat's great if we can make it Module Level only.
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)
Comment #55
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#54: Could you clarify the exact wording you would like, please?
Comment #56
droplet CreditAttribution: droplet commentedfor BEM-style naming standards
to
for Drupal's BEM-style naming standards
Comment #57
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commentedThanks for the reviews! I updated the patch #51.
1.
Surely it's unclear with the description. I added a note. Does this make it better?
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.
@droplet, I'm sorry, I don't understand what "Module Level only" means. Could you please explain a little more?
Comment #60
crizGreat 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:
Comment #61
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedBack to RTBC
Comment #62
droplet CreditAttribution: droplet commented@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.
Comment #64
stefan.r CreditAttribution: stefan.r commentedRe-reviewed with Fabianx, and committed and pushed to 7.x, thanks!
Comment #65
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #66
hgoto CreditAttribution: hgoto as a volunteer and at Studio Umi commentedGreat. Thank you!
Comment #68
jpamental CreditAttribution: jpamental at Isovera commentedNot 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?
Comment #69
hgoto CreditAttribution: hgoto as a volunteer commented@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...
Comment #70
artinruins CreditAttribution: artinruins commentedI 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.
Comment #71
criz@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!