Don't replace underscore with hyphen in CSS class names

With the help of this patch, it won't replace `_` with `-`. But in this patch, I'm passing some hardcoded strings filters ( ref: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util... ) excluding `_` to cleanCssIdentifier static method.

Instead of passing hardcoded strings filters , it would be better to give a checkbox option in the display setting where classes are being added to include '_' or not.
Because passing of static filters does't look good by using core APIs.Drupal generally uses "_" for machine name that's why it were removed..

can we have any other alternative to make it as per Drupal standard?

Hi @MahtabAlam There are fixed number of string filters ( check here https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Util...) i.e.

 ' ' => '-',
  '_' => '-',
  '/' => '-',
  '[' => '-',
  ']' => '',

That's why for now, I've passed all filters except `_` as a 2nd argument to the function.

Yes but other option can be creating a textfield, where we can give comma separated filters values. Then in code base side we can converts those values into array. And then pass it as 2nd arguments. But this need feature work and have to write test cases.

Thanks, @pradeepjha. This looks good to me

The call to Html::cleanCssIdentifier() has been introduced in commit 9080241b6aa20de49bd3027627f8d67579343dd0.

  // Add class for whole field.
  if (isset($field_display['third_party_settings']['field_formatter_class'])) {
    $variables['attributes']['class'][] = Html::escape($field_display['third_party_settings']['field_formatter_class']['class']);
    $class = $field_display['third_party_settings']['field_formatter_class']['class'];
    /** @var \Drupal\Core\Utility\Token $token */
    $token = \Drupal::service('token');
    /** @var \Drupal\Core\Entity\EntityInterface $object */
    $object = $variables['element']['#object'];
    $class = $token->replace($class, [$object->getEntityTypeId() => $object]);
    $variables['attributes']['class'][] = Html::cleanCssIdentifier($class);
  }

Before that commit, the underscores weren't replaced by hyphens.

IMO, the correct plan is:

• Restore the previous behavior
• In this or in a new branch, add a setting that allow users to decide which characters must be kept/removed

Thanks for working on this everybody. This is a very unfortunate, but very interesting bug.

The call to Html::cleanCssIdentifier() has been introduced in commit 9080241

Indeed. That method is the cause of underscores being replaced with hyphens. However the bug didn't actually happen until the next commit. Going by git bisect, the bug was introduced by 7d58d19. That seemed odd to me, so I looked at both issues.

commit 7d58d191cbd9c3972cc33da4cfd664e02494656b
Author: pookmish <pookmish@3059577.no-reply.drupal.org>
Date:   Sun Dec 9 22:16:46 2018 +0200

    Issue #3017781 by pookmish, alex_optim: Remove token class

commit 9080241b6aa20de49bd3027627f8d67579343dd0
Author: pookmish <pookmish@3059577.no-reply.drupal.org>
Date:   Mon Dec 3 19:18:01 2018 +0200

    Issue #2990781 by pookmish, pifagor, Romixua, Mile3, alex_optim: Token Support

Then I realized something strange. If using 9080241, the class is output twice! One preserves the underscore, and the other changes it to a hyphen. So if you put "custom_class" in the formatter settings, the output is custom_class custom-class. It turns out the commit for issue #2990781 wasn't the same as the patch submitted for the issue. So it needed another commit in a follow-up issue. Phew, that mystery is solved.

Recap. What has changed:

1. Before those two commits, the class name was cleaned-up using Html::escape().
2. Afterwards, it is cleaned-up by Html::cleanCssIdentifier()

However #2990781: Token Support was about adding token support only. The change to Html::cleanCssIdentifier() wasn't necessary for that, and was clearly out of scope for the issue.

Interestingly, HTML::cleanCssIdentifier() leaves a double-underscore intact. That's common in BEM-style classes. Sites using this module to inject a double-underscore class will not have been affected by this issue.

IMO, the correct plan is: Restore the previous behavior

Agreed. To my mind that means restoring the use of HTML::escape(), not tweaking HTML::cleanCssIdentifier() arguments.

The module usage statistics were useful. Luckily, they were still available all the way back to the day before the 8.x-1.2 release.

• The 8.x-1.1 release is a couple years old, and ~8000 sites report using it at the start of May 2020.
• Since the 8.x-1.2 release, approximately 2000 sites have updated. Some sites may have been affected by this issue, and modified their stylesheets as a workaround. In that case, returning to the old behaviour won't break their site again.
• Approx 6000 remain on the 8.x-1.1 release. That's fine; none of these were security updates, there's no need for anyone to update unless they need Drupal 9 compatibility. It's a good idea to assume that these sites will want to update to Drupal 9, in which case it will be good to avoid breaking their theme.

In this or in a new branch, add a setting that allow users to decide which characters must be kept/removed

No, I don't want to support that. This module only aims to provide simple formatter settings. The idea of an additional formatter setting, which controls what you're allowed to write in the nearby class text box doesn't make sense to me. If you don't need an underscore, just don't type it. I don't want to add a separate settings page either, for super-administrators to limit what lesser administrators can do. This module has been in use for a decade now, and that feature has never been asked for.

I've changed it back to using Html::escape() instead of Html::cleanCssIdentifier().

@pradeepjha I didn't use your patch, but you'll still get an issue credit.

There will be an 8.x-1.4 release very shortly.

Good catch

Add issue credit for:
Tusch001, kiamlaluno, andrewmacpherson