Get rid of strtr in Html::cleanCssIdentifier

Needs rebase.

Added beta eval

Thanks, RTBC!

I think in this case a comment would be worth it here

That works for me.

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -78,12 +78,17 @@ public static function getClass($class) {
+    $identifier = str_replace('__', '##', $identifier);
+    $identifier = str_replace(array_keys($filter), array_values($filter), $identifier);
+    $identifier = str_replace('##', '__', $identifier);

If the passed in $filter = ['__'=>'_']; this does does something different than before. Why is $filter configurable?

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -78,12 +78,17 @@ public static function getClass($class) {
     '_' => '-',
-    '__' => '__',

These both together is a pain in the ass, it's a legacy workaround for BEM support. I wonder how much damage it would be if we removed both?

I can open a new issue for that but just wanted to bring this up in light of the un-RTBC marked by it.

Addressing the concern in #16, I want to see what fails if we remove the $filter parameter. From what I could see in PHPStorm, nothing is using it. It is probably a BC break, but I want to see if something breaks in core.

The only things failing are situations we are checking only in tests. These are the two replacements that fail.

1) Drupal\Tests\Component\Utility\HtmlTest::testCleanCssIdentifier with data set #0 ('abcdefghijklmnopqrstuvwxyz_AB...456789', 'abcdefghijklmnopqrstuvwxyz_AB...456789', array())
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-abcdefghijklmnopqrstuvwxyz_ABCDEFGHIJKLMNOPQRSTUVWXYZ-0123456789
+abcdefghijklmnopqrstuvwxyz-ABCDEFGHIJKLMNOPQRSTUVWXYZ-0123456789

/var/www/d8task/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php:50

2) Drupal\Tests\Component\Utility\HtmlTest::testCleanCssIdentifier with data set #3 ('invalididentifier', 'invalid !"#$%&'()*+,./:;<=>?@...tifier', array())
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-invalididentifier
+invalid---identifier

/var/www/d8task/core/tests/Drupal/Tests/Component/Utility/HtmlTest.php:50

Both of these replacements are passed in with an empty array for filter, which means there should no replacements. I am not sure if this is a required use case.

Of course, another option is that we only do str_replace('__', '##',...) if it is not set in $filter. Does that sound workable?

I have undone patch #18 and wrapped the double underscore replacements in a conditional statement.

I have also added a test to ensure that $filter = ['__'=>'_']; works as expected - from #16

The other way to fix #18 would be to change the tests.

Either \Drupal\Tests\Component\Utility\HtmlTest::testCleanCssIdentifier() should also test for !empty($filter) or \Drupal\Tests\Component\Utility\HtmlTest::providerTestCleanCssIdentifier() should provide NULL as the third parameter in the arrays.

Also, removing $filter from the \Drupal\Component\Utility\Html::cleanCssIdentifier() definition would be an API change.

Just some more backup for this issue, noticed Twig is doing this change as well:
https://github.com/twigphp/Twig/pull/1818

Benchmark https://3v4l.org/PXWDg/perf#tabs vs https://3v4l.org/iuq1Z/perf#tabs

RTBC, looks great to me now.

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -78,12 +78,21 @@ public static function getClass($class) {
+    if (!isset($filter['__'])) {
+      $identifier = str_replace('__', '##', $identifier);
+    }
+    $identifier = str_replace(array_keys($filter), array_values($filter), $identifier);
+    if (!isset($filter['__'])) {
+      $identifier = str_replace('##', '__', $identifier);
+    }

Given this is about a micro-optimisation we can actually go further. str_replace() has another parameter which is a count of how many replacements made. So we could define a variable $double_underscore_replacements and pass it to the first str_replace() and then only do the final str_replace() if it is greater than 0.

Great idea.

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -78,12 +78,22 @@ public static function getClass($class) {
+    if (!isset($filter['__']) && $double_underscore_replacements > 0) {

The isset check here now is not needed.

True.

Changed the comment to be more specific as suggested in #33.

Added the documentation tag for the DrupalCon sprint.

Reviewing this at drupalcon barcelona

This looks good, liked the idea of alex :)

Committed 7ae23ca and pushed to 8.0.x. Thanks!