Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
Follow-up to #2031641: Change active class to is-active
core/modules/config_translation/src/Controller/ConfigTranslationEntityListBuilder.php:70
$row['label']['class'] = 'table-filter-text-source';
This should not be concatenating strings to create classes. It should be an array like all Attributes that are added to the Attribute object.
Proposed resolution
Convert concatenated string classes that build up a class attribute on the array object to an array.
Before
$class = '';
$class .= 'active';
$class .= ' next-class';
After
$classes = [];
$classes[] = 'active';
$classes[] = 'next-class';
Which the Attribute object will print out with no extra spaces and this is how D7 works as well with drupal_attributes().
Remaining tasks
None.
User interface changes
None.
API changes
None.
Beta phase evaluation
| Issue category | Task because classes should be an array, not concatenated strings. |
|---|---|
| Issue priority | Normal because the patch fixes class attribute assembly only in one place in the code. |
| Prioritized changes | Reduces fragility of the code. |
| Disruption | Not disruptive at all. |
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | attribute-string-cleanup-2414413-11.patch | 734 bytes | joelpittet |
| #11 | interdiff.txt | 888 bytes | joelpittet |
| #8 | attribute-string-cleanup-2414413-8.patch | 749 bytes | joelpittet |
Comments
Comment #1
idebr commentedI think the correct status is 'Active' since there is no patch attached to the issue. For a novice issue, I think the scope should be clarified and the proposed resolution should be more detailed. @joelpittet could you elaborate on the scope and proposed resolution?
Comment #2
joelpittet@idebr thanks you are right, it may be even a good idea to postpone it on that is-active parent issue, so it doesn't cause reroll/merge conflict.
Does the IS changes help?
Comment #3
joelpittetComment #4
idebr commentedYes, that helps a lot, thanks :)
Is this issue specifically about
SystemController::setLinkActiveClass()or is it an instance of a larger problem that should be fixed in this issue?Comment #5
Aki Tendo commentedWorking on a new class as part of the Template Assertion project, which involves adding assertions to and general dev hardening of the Attribute family of classes. That class is AttributeNamedClass which extends off AttributeValueBase and holds all manner of CSS Class manipulation. It will still accept a string argument but explodes it on spaces to form an array internally which is then used to track for duplicates. I don't know how much of this issue it addresses (if any)
Comment #6
star-szrThanks @Aki Tendo for cluing me into an older and very related issue :)
Also un-postponing, I don't think a small merge conflict is enough to postpone, at least not for this long.
Comment #7
joelpittetWow, looked at the code for the one I was hoping to change and it's changed a bunch, it's now a DOMDocument object magic thing... so yeah.
@idebr it's a bigger issue to hunt down any lingering string concatinations of CSS classes and convert them to arrays.
Since that one in the IS is not a good candidate I've looked through to spot some:
Easiest is to look for this string:
['class'] = 'incore/. You may find others too and some false friends where class refers to a real PHP class and not CSS class so it can be tricky that way.New example:
core/modules/config_translation/src/Controller/ConfigTranslationEntityListBuilder.php:70Comment #8
joelpittetI did a thorough search through core and that's the only one left, so nice... time heals all wounds;)
This just needs to be converted to
#attributeskey as well for the table cell. 'class' is no longer a thing for the#type=>tableComment #9
joelpittetMaybe someone can do a sweep behind and see if I've missed anything?
Comment #11
joelpittetWhoops, it's building it for preprocess_table not type table because it's attaching directly to
#rowskey, which still can take an array or string... but for consistency let's keep it as an array. Type table DX is bizarre...Comment #12
joelpittetComment #13
davidhernandezIs this really all that is needed? Only change instances where we're building CSS classes? I grepped around a bit too and didn't notice anything more than the one Joel found.
Comment #14
joelpittet@davidhernandez yes I believe so, it was much worse off before but seems things got cleaned up as we went.
Comment #15
bbujisic commentedI also did not notice anything more. Patch #12 is RTBC.
Comment #16
alexpottIs this a bug? Should we stop it from working? If this is a normal task then we need a beta evaluation.
Comment #17
Aki Tendo commentedAlso, there is crossover, at least conceptually, with what I'm doing in #244003: Site stuck on theme - doesn't update, please help which transfers the management of the class attribute to a specialized class instead of having that logic drifting around in the Attribute and AttributeValueArray classes. That needs to be checked.
Comment #18
bbujisic commentedI've added the Beta phase evaluation table to the issue.
Comment #19
alexpottFrom #16 has still not been answered.
Comment #20
lewisnymanWe discussed this during Drupal Dev Days and agreed that it is a task, not a bug, as there is no reproducible problem but is related to our coding standards.
Commit message:
git commit -m 'Issue #2414413 by joelpittet, Branislav Bujisic: Make sure we are building CSS classes as arrays'Comment #21
Aki Tendo commentedLooking at the patch --
- $row['label']['class'] = 'table-filter-text-source';
+ $row['label']['class'][] = 'table-filter-text-source';
To the AttributeNamedClass class (awkward name, I know) in #2444003: Optimize Drupal\Core\Template\Attribute there is no effective difference between these two syntaxes. I'm not suggesting changing this since it makes the programmer's intent clear. (This is assuming that $row['label'] is an Attribute object).
Comment #22
alexpottCommitted 8048a38 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.