Right now, core uses form_clean_id() in a few places to clean perspective class names before inserting them into the html. But form_clean_id() is the wrong function to clean class names because it enforces a uniqueness to them by appending _0, _1, etc if form_clean_id() has been previously called with the same input.

http://www.w3.org/TR/CSS21/syndata.html#characters shows the syntax for valid class names and IDs.

In CSS, identifiers (including element names, classes, and IDs in selectors):

  1. can contain only the characters [a-zA-Z0-9] (U+0030 - U+0039, U+0041 - U+005A, U+0061 - U+007A) and ISO 10646 characters U+00A1 and higher, plus the hyphen (U+002D) and the underscore (U+005F)
  2. cannot start with a digit, or a hyphen followed by a digit.
  3. can also contain escaped characters and any ISO 10646 character as a numeric code (see next item). For instance, the identifier "B&W?" may be written as "B\&W\?" or "B\26 W\3F".

I'm all for ignoring the 3rd part of that spec (too messy), but we should definitely implement the first 2 parts.

In addition to the validity of classes, we should also look to Drupal’s code style. We currently don't use underscores in class names, so this new function should replace them with dashes by default, but allow callers to relax that restriction if they have a use case (like integrating with 3rd party code.)

CommentFileSizeAuthor
#61 464862-govuk-frontend-exemption-61.patch1.17 KBcodebymikey
#53 464862-53-drupal-css-class.patch6.51 KBJohnAlbin
#51 464862-51-drupal-css-class.patch6.54 KBJohnAlbin
#50 464862-50-drupal-css-class.patch6.35 KBJohnAlbin
#47 b69d64b4.patch14.03 KBkkaefer
#43 b69d64b4.patch11.13 KBkkaefer
#39 ab-head.txt1.3 KBsun
#39 ab-patch.txt1.3 KBsun
#35 drupal.css-class.33.patch18.41 KBdawehner
#32 drupal.css-class.32.patch20.11 KBsun
#29 drupal-css-class-464862-29.patch17.38 KBJohnAlbin
#21 drupal-css-class-464862-21.patch19.28 KBJohnAlbin
#19 drupal-css-class-464862-19.patch19.29 KBJohnAlbin
#17 drupal-css-class-464862-17.patch18.51 KBJohnAlbin
#12 drupal-css-class-464862-12.patch21.71 KBJohnAlbin
#10 drupal-css-class-464862-10.patch20.77 KBJohnAlbin
#9 drupal-css-class-464862-9.patch15.39 KBJohnAlbin
#8 drupal-css-class-464862-8.patch15.13 KBJohnAlbin
#6 drupal-css-class-464862-5.patch12 KBJohnAlbin
#4 drupal-css-class-464862-4.patch11.98 KBJohnAlbin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

subscribing

Handle renaming of form_clean_id() in here as well?

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Title: Add drupal_css_class() to clean class names instead of form_clean_id » Add drupal_css_class() to clean class names and rename form_clean_id

Sure! +1 to consistent api names.

dvessel’s picture

dvessel CreditAttribution: dvessel commented

subscribing

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Active » Needs review
FileSize
drupal-css-class-464862-4.patch11.98 KB

Here's a completely untested patch (I mean it!) that implements a rough-draft approach to drupal_css_class() and drupal_css_id().

Comments/improvements on the implementation are appreciated.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Needs review
FileSize
drupal-css-class-464862-5.patch12 KB

bah. \u is only supported in Perl and not PHP. :-p

New patch attached. Still mostly untested.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Needs review
FileSize
drupal-css-class-464862-8.patch15.13 KB

Missed some simpletests for drupal_css_id().

I still need to add some tests that verify that drupal_css_class() works properly.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
FileSize
drupal-css-class-464862-9.patch15.39 KB

re-rolled.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
FileSize
drupal-css-class-464862-10.patch20.77 KB

Here's what this patch does:

  1. Implements a drupal_css_class() in common.inc using the rules specified at the top of this issue. See the patch for the details of the function parameters; drupal_css_class() is heavily documented.
  2. Renames form_clean_id() to drupal_css_id() and moves it from form.inc to common.inc.
  3. Modifies drupal_css_id() so that it uses the same validation rules as drupal_css_class(). (It does this by using drupal_css_class() as a helper function.)
  4. Changes non-unique IDs so that the second non-unique ID is appended with "-2" instead of "-1" the way form_clean_id() worked. So a non-unique ID sequence would look like this: form-example, form-example-2, form-example-3.
  5. Modified drupal_attributes() so that each "class" is validated.
  6. Adds simpletests for drupal_css_class() and drupal_css_id() and removes the old form_clean_id() tests.

The patch is now feature complete. But implementation details are still open for debate, as always! :-)

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

Some of the code in the patch is over my head, but based on the summary in #10, +1 ;)

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Issue tags: +theme, +Needs design review, +tpl-refresh, +Needs themer review
FileSize
drupal-css-class-464862-12.patch21.71 KB

After reviewing this, I realized that drupal_css_id() should have the same params as drupal_css_class(). Also, using drupal_css_class() as a helper function for drupal_css_id() started to feel kind of weird, so I've introduced a proper helper function, drupal_clean_css_identifier().

So that we can get some more reviews of these 3 new functions (from people who don't necessarily know how to apply patches), here's the docblock docs for them:

/**
 * Prepare a string for use as a valid CSS identifier (element, class or ID selector).
 *
 * http://www.w3.org/TR/CSS21/syndata.html#characters shows the syntax for valid
 * CSS identifiers (including element names, classes, and IDs in selectors.)
 *
 * @param $identifier
 *   The CSS identifier to clean.
 * @param $coding_standards
 *   Enforce Drupal's coding standards on the identifier string.
 * @param $safe_prefix
 *   If the proposed identifier starts with an illegal sequence (a digit or a
 *   dash followed by a digit), prepend a safe string to the identifier;
 *   defaults to 'css-'.
 * @return
 *   The cleaned identifier.
 */
function drupal_clean_css_identifier($identifier, $coding_standards = TRUE, $safe_prefix = 'css-') {
  // Valid characters 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.
  $identifier = preg_replace('/[\x{0000}-\x{002C}\x{002E}-\x{002F}\x{003A}-\x{0040}\x{005B}-\x{005E}\x{0060}\x{007B}-\x{00A0}]/u', '', $identifier);

  // Drupal's coding standards use lowercase characters and dashes instead of
  // underscores.
  if ($coding_standards) {
    $identifier = str_replace('_', '-', drupal_strtolower($identifier));
  }

  // CSS identifiers can't start with a number or a dash and a number, so we
  // prepend a safe string to prevent an invalid identifier.
  $start = substr($identifier, 0, 1);
  if ($start == '-') {
    $start = substr($identifier, 1, 1);
  }
  if (is_numeric($start)) {
    $identifier = $safe_prefix . $identifier;
  }

  return $identifier;
}

/**
 * Prepare a string for use as a valid CSS class name.
 *
 * @param $class
 *   The class name to clean.
 * @param $coding_standards
 *   Enforce Drupal's coding standards on the class name.
 * @param $safe_prefix
 *   If the proposed class name starts with an illegal sequence (a digit or a
 *   dash followed by a digit), prepend a safe string to the class name;
 *   defaults to 'class-'.
 * @return
 *   The cleaned class name.
 */
function drupal_css_class($class, $coding_standards = TRUE, $safe_prefix = 'class-') {
  return drupal_clean_css_identifier($class, $coding_standards, $safe_prefix);
}

/**
 * Prepare a string for use as a valid HTML ID and guarantee uniqueness.
 *
 * @param $id
 *   The ID to clean.
 * @param $coding_standards
 *   Enforce Drupal's coding standards on the ID string.
 * @param $safe_prefix
 *   If the proposed ID starts with an illegal sequence (a digit or a dash
 *   followed by a digit), prepend a safe string to the ID; defaults to 'id-'.
 * @return
 *   The cleaned ID.
 */
function drupal_css_id($id, $coding_standards = TRUE, $safe_prefix = 'id-') {
  $seen_ids = &drupal_static(__FUNCTION__, array());
  $id = str_replace(array('][', '_', ' '), '-', $id);
  $id = drupal_clean_css_identifier($id, $coding_standards, $safe_prefix);

  // Ensure IDs are unique. The first occurrence is held but left alone.
  // Subsequent occurrences get a number appended to them. This incrementing
  // will almost certainly break code that relies on explicit HTML IDs in forms
  // that appear more than once on the page, but the alternative is outputting
  // duplicate IDs, which would break JS code and XHTML validity anyways. For
  // now, it's an acceptable stopgap solution.
  if (isset($seen_ids[$id])) {
    $id = $id . '-' . ++$seen_ids[$id];
  }
  else {
    $seen_ids[$id] = 1;
  }

  return $id;
}

drupal_css_id() is almost identical to the old form_clean_id() but with 2 lines changed as I noted in #10 above.

Zarabadoo’s picture

Zarabadoo CreditAttribution: Zarabadoo commented

This all looks good to me in terms of the final goal. I can definitely see myself using this set of functions in the future if it is implemented.

dvessel’s picture

dvessel CreditAttribution: dvessel commented

@JohnAlbin, do you think there would be any performance issues with preg_replace being hit so often? Having valid output is great but are we being overly cautious? The only time core uses preg_replace is for an attribute string AFAIK is in cleaning out the url to be converted to a body class. It never seemed to be an issue for form_clean_id(). Anything invalid was predictable and we filtered for that.

About the use of the $coding_standard parameter. Instead of using a binary switch, what if you could pass in a string or an array to be filtered.

example:

/**
 * ...
 * @param
 *   (optional) A string or an array of strings to filter out.
 *   It will be converted into hyphens.
 */
function drupal_clean_css_identifier($identifier, $filters = '_', $safe_prefix = 'css-') {
  ...

  // Drupal's coding standards use lowercase characters and dashes.
  if (!empty($filters) {
    $identifier = str_replace($filters, '-', drupal_strtolower($identifier));
  }

  ...

  return $identifier;
}

With that, you can have any string converted not just underscores. The default behavior would result in the same output. It's just more flexible.

This could possibly replace the need for preg_replace() by feeding the filter with expected "invalid" values as they are encountered.

What the id cleaner could look like:

function drupal_css_id($id, $filter = array(array('][', '_', ' '), $safe_prefix = 'id-') {
  $seen_ids = &drupal_static(__FUNCTION__, array());
  $id = drupal_clean_css_identifier($id, $filter, $safe_prefix);

  ...

  return $id;
}

Lastly, is $safe_prefix necessary? I've never encountered a situation where I had to be careful of an invalid prefix but maybe others have.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work

We badly and definitely need this.

However, some clean-up is necessary:

 function drupal_attributes($attributes = array()) {
   if (is_array($attributes)) {
     $t = '';
+    // Handle classes specially.
+    if (isset($attributes['class'])) {
+      // Clean the class names, but don't enforce Drupal coding standards since
+      // we may need to integrate with 3rd party APIs.
+      $classes = explode(' ', $attributes['class']);
+      if (!empty($classes)) {
+        foreach ($classes AS $i => $class) {
+          $classes[$i] = drupal_css_class($class, FALSE);
+        }
+        $t .= ' class="' . implode(' ', $classes) . '"';
+      }
+      unset($attributes['class']);
+    }
     foreach ($attributes as $key => $value) {
       $t .= " $key=" . '"' . check_plain($value) . '"';
     }

'class' is special-cased here, but $attributes can as well contain a CSS ID. We want to ensure both are sanitized.

Additionally, that isset() and following unset() is a bit clumsy. Please move that as checks for the two special values in $key into the foreach.

+ * @param $coding_standards
+ *   Enforce Drupal's coding standards on the identifier string.

That is a pretty nifty idea. The parameter/variable name might use a shorter name though. And a link to the handbook page where these are defined would be great (if existent).

+ * @param $safe_prefix
+ *   If the proposed identifier starts with an illegal sequence (a digit or a
+ *   dash followed by a digit), prepend a safe string to the identifier;
+ *   defaults to 'css-'.

Please remove this entire option. With or without it, developers and themers won't get the expected result, so there is no point in putting extra logic in there. We just want to strip anything invalid at the beginning in all cases.

+  $identifier = preg_replace('/[\x{0000}-\x{002C}\x{002E}-\x{002F}\x{003A}-\x{0040}\x{005B}-\x{005E}\x{0060}\x{007B}-\x{00A0}]/u', '', $identifier);

A negated, positive char list would seem more natural to me here, no? I.e.

+  $identifier = preg_replace('/[^\x{002D}\x{0030}-\x{0039}\x{0041}-\x{005A}\x{005F}\x{0061}-\x{007A}\x{00A1}-]/u', '', $identifier);

...

+    $identifier = str_replace('_', '-', drupal_strtolower($identifier));

Should use strtr() here.

+  // CSS identifiers can't start with a number or a dash and a number, so we
+  // prepend a safe string to prevent an invalid identifier.
+  $start = substr($identifier, 0, 1);
+  if ($start == '-') {
+    $start = substr($identifier, 1, 1);
+  }
+  if (is_numeric($start)) {
+    $identifier = $safe_prefix . $identifier;
+  }

That probably needs another preg_replace(), i.e.

  $identifier = preg_replace('^[\d-]', '', $identifier);

However, in general, I'm not completely sold on the idea that Drupal should all of this extra work due to sluggish developers. AFAIK, Drupal does not do this elsewhere. CSS IDs were the only special case, because those can't appear twice on a page - while a single developer is not able to control with other CSS IDs are on a page.

So. Bearing that, I would suggest to remove that extra validation for starting numbers/dashes.

The remaining clean-up of form_clean_id() looks just awesome!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I wonder whether we should move the code from #550572: CSS+JS regressions related to form-item-[name] into drupal_css_class() after that has been committed.

Extract:

  if (!empty($element['#name'])) {
    $class[] = 'form-item-' . strtr($element['#name'], array(' ' => '-', '_' => '-', '[' => '-', ']' => ''));

The not-so-obvious trick lies in ] being replaced with an empty string, and spaces, underscores, and [ being replaced with hyphens. That means:

book_parent[bid][title] becomes book-parent-bid-title

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Needs review
FileSize
drupal-css-class-464862-17.patch18.51 KB

do you think there would be any performance issues with preg_replace being hit so often?

Hmm… I really don’t know.

A negated, positive char list would seem more natural to me here, no? I.e.

  $identifier = preg_replace('/[^\x{002D}\x{0030}-\x{0039}\x{0041}-\x{005A}\x{005F}\x{0061}-\x{007A}\x{00A1}-]/u', '', $identifier);

That's true, but the end of your pattern doesn't work. \x{00A1}- doesn't mean character A1 and higher, it means \x{00A1} and -. That was why I had originally written it the more awkward way. But thinking about this again, we could use \x{00A1}-\x{FFFF}.

$identifier = str_replace('_', '-', drupal_strtolower($identifier));
Should use strtr() here.

Really? They are functionally equivalent in this instance, but strtr is slower. See http://www.simplemachines.org/community/index.php?topic=175031.0;wap2 Oh, wait. I guess not when using single characters. See http://www.cznp.com/blog/3/strtr-vs-str_replace-a-battle-for-speed-and-d... too. :-p So confusing! I'll replace it with strtr().

So. Bearing that, I would suggest to remove that extra validation for starting numbers/dashes.

Yeah, ok. I agree. I tend to get all “feature complete-y” when coding. :-)

I wonder whether we should move the code from #550572: CSS+JS regressions related to form-item-[name] into drupal_css_class()

strtr($element['#name'], array(' ' => '-', '_' => '-', '[' => '-', ']' => ''));

Ok, that makes me think we should implement Joon’s idea of a $filter parameter using that format.

This new patch simplifies drupal_css_class() and drupal_css_id(). They only have the $identifier paramter and always use Drupal’s coding standards as a filter. If someone needs to implement a non-Drupal, 3rd-party class or ID, they can access the underlieing drupal_clean_css_identifier() which uses the $filter parameter.

Also, I realized we can't really run drupal_clean_css/id from inside drupal_attributes(). Because that would prevent developers from being able to go around Drupal coding standards if they needed to. So we just need to run drupal_css_class/drupal_css_id before strings are added to a $attributes array.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Needs review
FileSize
drupal-css-class-464862-19.patch19.29 KB

Fixed broken test.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented 
+++ includes/common.inc	27 Aug 2009 20:20:52 -0000
@@ -2817,6 +2817,79 @@ function drupal_clear_css_cache() {
+ * incorrectly concatenated with dashes. i.e. "one two" will become "one-two".

Comma before i.e.

+++ includes/form.inc	27 Aug 2009 20:20:54 -0000
@@ -625,7 +625,7 @@ function drupal_prepare_form($form_id, &
-      '#id' => form_clean_id('edit-' . $form_id . '-form-token'),
+      '#id' => drupal_css_id('edit-' . $form_id . '-form-token'),
@@ -635,11 +635,11 @@ function drupal_prepare_form($form_id, &
-      '#id' => form_clean_id("edit-$form_id"),
+      '#id' => drupal_css_id("edit-$form_id"),
...
   if (!isset($form['#id'])) {
-    $form['#id'] = form_clean_id($form_id);
+    $form['#id'] = drupal_css_id($form_id);
   }
@@ -993,7 +993,7 @@ function form_builder($form_id, $element
   if (!isset($element['#id'])) {
-    $element['#id'] = form_clean_id('edit-' . implode('-', $element['#parents']));
+    $element['#id'] = drupal_css_id('edit-' . implode('-', $element['#parents']));
   }
@@ -1867,7 +1867,7 @@ function form_process_radios($element) {
-          '#id' => form_clean_id('edit-' . implode('-', $parents_for_id)),
+          '#id' => drupal_css_id('edit-' . implode('-', $parents_for_id)),
@@ -2183,7 +2183,7 @@ function form_process_tableselect($eleme
-            '#id' => form_clean_id('edit-' . implode('-', $parents_for_id)),
+            '#id' => drupal_css_id('edit-' . implode('-', $parents_for_id)),

We should consider to remove some of those auto-generated IDs. They cannot be used safely anyway. And since we have shiny new + clean form element CSS classes now, they are even more obsolete.

Different issue though. ;)

+++ modules/simpletest/tests/common.test	27 Aug 2009 20:21:01 -0000
@@ -455,6 +455,59 @@ class CascadingStylesheetsTestCase exten
+      'name' => t('CSS identifiers'),
+      'description' => t('Test the functions drupal_css_class() and drupal_css_id() for expected behavior'),
+      'group' => t('System')

We no longer wrap these in t(). Also, missing trailing comma after last element.

Beer-o-mania starts in 4 days! Don't drink and patch.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
FileSize
drupal-css-class-464862-21.patch19.28 KB

Re-rolled with Sun's suggestions.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Thanks! Let's get this wonderful patch in! :)

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Issue tags: +API change, +API addition

tagging

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

lilou’s picture

lilou CreditAttribution: lilou commented
Status: Needs work » Reviewed & tested by the community

HEAD is broken.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

That bit about formatting stuff according to Drupal coding standards is cruft, IMO. Let's strip it out; this is the domain of Coder module. Core doesn't babysit broken code.

It would also be good to get some before/after benchmarks for that preg_replace().

Other than that, this looks great!

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Reviewed & tested by the community

After talking to webchick in the hallway, I think she was reviewing the patch in #12. Yes, the $coding_standards parameter in that patch was goofy. That's why the patch in #21 nixed it.

Setting back to RTBC to let webchick have another look.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Reviewed & tested by the community
FileSize
drupal-css-class-464862-29.patch17.38 KB

“Failed: Failed to run tests.”? Hmm… the patch in #21 still applied (with fuzz).

Re-rolling and letting the testbot have another try.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I confirm this is RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Reviewed & tested by the community
FileSize
drupal.css-class.32.patch20.11 KB

Straight re-roll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs work » Needs review
FileSize
drupal.css-class.33.patch18.41 KB 
+++ includes/theme.inc	19 Sep 2009 03:37:03 -0000
@@ -2113,18 +2113,17 @@ function template_preprocess_html(&$vari
   if ($node = menu_get_object()) {
-    $variables['classes_array'][] = 'node-type-' . form_clean_id($node->type);
+    $variables['classes_array'][] = drupal_css_class('node-type-' . $variables['node']->type);

Here was the problem.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

yay, thanks! :)

Reverting status.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: -Needs design review, -Needs themer review +API clean-up, +D7 code freeze

Tagging.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

*bump*

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
ab-patch.txt1.3 KB
ab-head.txt1.3 KB

Is it because of the missing benchmarks, maybe?

Attached. No noticeable difference.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Reviewed. Looks good and convenient. Committed to CVS HEAD. Thanks all!

kkaefer’s picture

kkaefer CreditAttribution: kkaefer commented

Minor nitpick: It's technically not a "CSS" class or ID but rather a "HTML" class or ID, so drupal_css_class() and drupal_css_id() are kind of a misnomer. IDs and classes are not inherently tied to CSS but are HTML attributes (http://www.w3.org/TR/html4/struct/global.html#h-7.5.2).

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

@kkaefer: Your nitpick makes sense -- do we need to fix it or would the current function names be more grokable than drupal_html_class() and drupal_html_id()?

kkaefer’s picture

kkaefer CreditAttribution: kkaefer commented
Status: Fixed » Needs review
FileSize
b69d64b4.patch11.13 KB

Patch changes drupal_css_* to drupal_html_*

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Reviewed & tested by the community

Supporting that.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Reviewed & tested by the community » Needs review

But why did we leave out drupal_clean_css_identifier() ?

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented

Seems reasonable.

kkaefer’s picture

kkaefer CreditAttribution: kkaefer commented
FileSize
b69d64b4.patch14.03 KB

also rename drupal_clean_css_identifier().

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community

Yup! Test bot likes it too.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Hm. I could go either way on this. I like "css" being in the name since that's a good clue to people about when to use those functions, but at the same time kkaefer's right that technically these are not inherent at all to CSS and can be used for other things as well.

I decided to go with accuracy/future-proofing and committed to HEAD.

Needs documentation (again). :)

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Needs review
FileSize
464862-50-drupal-css-class.patch6.35 KB

Well, if we’re going to nitpick… ;-)

IDs and classes are not inherently tied to CSS but are HTML attributes (http://www.w3.org/TR/html4/struct/global.html#h-7.5.2).

Agreed.

However, (annoyingly) the HTML spec has different constraints on what are valid classes or IDs than what the CSS spec has. The CSS spec is described in this issue’s original description above.

The HTML spec for HTML identifiers shows that:

  • class is defined to be CDATA (http://www.w3.org/TR/html4/types.html#h-6.2). This is much less strict than the CSS spec. So drupal_html_class() would incorrectly? strip characters that were valid per the HTML spec. Of course, I don’t know a real world use of the class attribute besides CSS, so this may be moot.
  • id is defined as an SGML name (http://www.w3.org/TR/html4/types.html#h-6.2). This is much more strict than the CSS spec. So drupal_html_class() would incorrectly leave characters that were invalid per the HTML spec. Basically, Unicode characters and most special characters are valid in CSS IDs, but not in HTML IDs.

So, IMO, we have a few reasonable solutions to this:

  1. Since the code cleans the identifiers per the CSS spec, rename the functions back to drupal_css_*. (i.e. revert the patch in #47).
  2. Change drupal_html_id() so that it uses a character-stripping pattern that matches the HTML spec and is stricter than the one provided in drupal_clean_html_identifier(). And rename drupal_clean_html_identifier() back to drupal_clean_css_identifier(), since the HTML spec has different constraints depending on whether its an element, id, or class name and, therefore, we haven't really implemented a function that cleans HTML identifiers.
  3. Keep the function names the same, but better document all three functions to show that it doesn't follow the HTML spec for valid identifiers.

I'm leaning to #2 and, since its easy to roll a patch for it, I've done so. But, consensus would be nice. My patch also fixes up some comments/naming in the tests.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
FileSize
464862-51-drupal-css-class.patch6.54 KB

Here's a new patch with an update to testDrupalHTMLId() that adds a "Strip invalid characters" check.

Status: Needs review » Needs work

The last submitted patch failed testing.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented
Status: Needs work » Needs review
FileSize
464862-53-drupal-css-class.patch6.51 KB

Fixed the testDrupalHTMLId() test to work with the updated drupal_html_id().

RobLoach’s picture

RobLoach
he/him
CreditAttribution: RobLoach commented
Status: Needs review » Reviewed & tested by the community

Reasonable!

In summary:

  • drupal_clean_html_identifier --> drupal_clean_css_identifier
  • DrupalCSSIdentifierTestCase --> DrupalHTMLIdentifierTestCase
  • Adds in a fix to respect the HTML ID specifications more
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

Haha. You guys are hilarious. :)

Cool, works for me. Committed to HEAD once more. Marking needs work for documentation.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue tags: -API change, -API addition, -API clean-up, -D7 code freeze

.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue tags: -Needs documentation +Needs documentation updates

Ummm... Looks like it needs documentation in the module update guide? changing tag scheme.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Fixed

OK. I'm looking through the 3 patches above that were committed. What I think happened in this issue:

First patch http://drupal.org/files/issues/drupal.css-class.33.patch
a) form_clean_id() [D6 function] was removed and replaced by drupal_css_id()
b) drupal_css_class() was added.
c) drupal_clean_css_identifier() was added.

Second patch http://drupal.org/files/issues/b69d64b4_1.patch
a) drupal_clean_css_identifier -> drupal_clean_html_identifier [later reverted see below]
b) drupal_css_class -> drupal_html_class
c) drupal_css_id -> drupal_html_id

Third patch http://drupal.org/files/issues/464862-53-drupal-css-class.patch
a) drupal_clean_html_identifier -> drupal_clean_css_identifier

Net result:
a) form_clean_id() [D6 function] -> drupal_html_id()
b) drupal_css_class() was added.
c) drupal_clean_html_identifier() was added.

(a) needs to be documented in the Update Guide. Actually, it was documented, but they didn't get the later change, so I fixed it:
http://drupal.org/update/modules/6/7#form_clean_id

Status: Fixed » Closed (fixed)
Issue tags: -theme, -tpl-refresh, -Needs documentation updates

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

Issue tags: +theme, +tpl-refresh, +Needs documentation updates

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

codebymikey’s picture

codebymikey CreditAttribution: codebymikey at Zodiac Media commented
FileSize
464862-govuk-frontend-exemption-61.patch1.17 KB

Currently using a govuk based theme, and it requires support for the exclamation mark within the class names.

Attaching a relevant patch here for reference purposes.