While scanning the Drupal codebase for easy tasks I found that the drupal_html_class function is deprecated and has a pretty easy replacement. You can replace nearly all uses of drupal_html_class with \Drupal\Component\Utility's getClass() and in fact drupal_html_class is now a light wrapper around that function (passes the class string directly to the getClass function).
drupal_html_class is a function that ensures clean class names are passed to the rendering system. There may be a few places where it is not needed. That might be an important finding given that it has been reported in the past that this kind of class cleaning mechanism slows down the performance of themes: #680022: template_preprocess() is too slow in generating valid CSS class name
So here's what needs to be done.
1. Find every usage of drupal_html_class and drupal_clean_css_identifier
2. Replace with Html::getClass or Html::cleanCssIdentifier() and ensure the appropriate use statement is present to register the \Drupal\Component\Utility file.
3. Ensure drupal_html_class and drupal_clean_css_identifier has been replaced in Documentation too.
Comment | File | Size | Author |
---|---|---|---|
#14 | remove_usage_drupal_html_class-2382543-14.patch | 31.98 KB | ianthomas_uk |
Comments
Comment #1
rpayanmComment #2
rpayanmComment #3
rpayanmComment #4
cosmicdreams CreditAttribution: cosmicdreams commentedlooks that covers everything but the removal of drupal_html_class itself. Let's see if it passes.
Comment #5
cosmicdreams CreditAttribution: cosmicdreams commentedWhen I was looking into doing this, this was the only line that caused me pause.
Do we really want to clean a full string that's already clean?
Do we have this line because we want to promote to other developers that WHATEVER you use for this string (whether it's combined through a series of variables or a string) that it needs be cleaned by some other function?
Comment #6
ianthomas_ukI agree with #5, that is unnecessary, and we have loads of places where we just add a hard-coded string to ['class'].
There are also lots of calls to drupal_clean_css_identifier(), which is basically the same function but without static caching and strtolower. Given the patch is only 20k I think it would be worth doing those too (it's also deprecated).
Comment #7
rpayanmComment #8
ianthomas_ukLooks good, there was just one instance that was missed, which I've fixed in the attached patch.
Other than that I'd mark it RTBC, so @rpayanm if you're happy with my change I think you'd be allowed to mark it RTBC (other reviewers welcome too of course).
Comment #9
rpayanmOhhh yes! I searched for
drupal_html_class(
anddrupal_clean_css_identifier(
. Nice catch. Looks good for me :)Comment #10
mpdonadioI read the patch, and it looks like a good one-to-one replacement. Not changing status, but a few nits
Since things are being touched, though, there is some inconsistent usage of the method. Other places have all of the class parts in the method call. Consistent use may be a good idea.
This should probably use the $arguments parameter to ->xpath() and single quotes.
Comment #11
ianthomas_uk#10.1 The function takes an unknown string and converts it to a valid class name. We already know 'path-' is a valid (partial) class name, so there's no need to do the conversion on that. The function uses a static cache, so by not passing that partial you may even get a higher cache hit rate. I don't imagine the performance impact is huge, but these are functions that might be called a lot. Either way it's not something I'd personally worry about changing without a good reason.
#10.2 Yeah I guess that's technically better
Comment #12
mpdonadio#11-1, I get that. It was more a comment that the usage is inconsistent:
String concatenation outside method invokation.
String concatenation as part of method invokation.
It's just a nit, though; I should have posted both examples in my first comment.
Comment #14
ianthomas_ukReroll, just one easy conflict resolved:
Comment #15
alexpottCommitted fdec77f and pushed to 8.0.x. Thanks!
This issue is a prioritized change (removal of deprecated function use) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption.
This use was not used. Fixed on commit.