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

Reference: https://www.drupal.org/core/beta-changes
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.
CommentFileSizeAuthor
#12 attribute-string-cleanup-2414413-11.patch734 bytesjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,419 pass(es). View
#11 interdiff.txt888 bytesjoelpittet
#8 attribute-string-cleanup-2414413-8.patch749 bytesjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,810 pass(es), 0 fail(s), and 26 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

idebr’s picture

Status: Needs review » Active

I 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?

joelpittet’s picture

Issue summary: View changes
Status: Active » Postponed

@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?

joelpittet’s picture

Issue summary: View changes
idebr’s picture

Yes, 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?

Aki Tendo’s picture

Working 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)

Cottser’s picture

Thanks @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.

joelpittet’s picture

Issue summary: View changes

Wow, 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'] = ' in core/. 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:70

  $row['label']['class'] = 'table-filter-text-source';
joelpittet’s picture

Status: Active » Needs review
FileSize
749 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,810 pass(es), 0 fail(s), and 26 exception(s). View

I 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 #attributes key as well for the table cell. 'class' is no longer a thing for the #type=>table

joelpittet’s picture

Maybe someone can do a sweep behind and see if I've missed anything?

Status: Needs review » Needs work

The last submitted patch, 8: attribute-string-cleanup-2414413-8.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
888 bytes

Whoops, it's building it for preprocess_table not type table because it's attaching directly to #rows key, which still can take an array or string... but for consistency let's keep it as an array. Type table DX is bizarre...

joelpittet’s picture

FileSize
734 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,419 pass(es). View
davidhernandez’s picture

Is 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.

joelpittet’s picture

@davidhernandez yes I believe so, it was much worse off before but seems things got cleaned up as we went.

bbujisic’s picture

Status: Needs review » Reviewed & tested by the community

I also did not notice anything more. Patch #12 is RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Is this a bug? Should we stop it from working? If this is a normal task then we need a beta evaluation.

Aki Tendo’s picture

Also, there is crossover, at least conceptually, with what I'm doing in [#244003] 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.

bbujisic’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

I've added the Beta phase evaluation table to the issue.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Is this a bug? Should we stop it from working?

From #16 has still not been answered.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

We 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'

Aki Tendo’s picture

Looking 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).

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8048a38 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 8048a38 on 8.0.x
    Issue #2414413 by joelpittet: Make sure we are building CSS classes as...

Status: Fixed » Closed (fixed)

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