Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Originally from https://github.com/dpi/courier/issues/39 .
Bootstrap fails when a render elements #class attribute is a string. Drupal core will auto cast a #class from string to array, so Bootstrap should do the same.
Snippet from Drupal core doing cast:
\Drupal\Core\Template\Attribute
if ($name == 'class' && !is_array($value)) {
// Cast the value to string in case it implements MarkupInterface.
$value = [(string) $value];
}
Snippet from Bootstrap where it fails:
\Drupal\bootstrap\Utility\Attributes
public function &getClasses() {
$classes = &$this->offsetGet('class', []);
$classes = array_unique($classes);
return $classes;
}
Fails because array_unique()
requires an array, but it is a string.
This can be reproduced by creating a render element where its 'class' attribute is a string, eg:
$element = [
'#type' => 'container',
'#attributes' => [
'class' => 'foobar',
],
],
];
Comments
Comment #2
markhalliwellUsing arrays has been a "Best Practice™" since D7:
https://www.drupal.org/update/modules/6/7#class_attribute_array
The reason for this is because arrays allow easier manipulation of classes. This has been the de facto format for how to define classes... for years.
Despite the fact that core technically "allows" strings by casts them into an array isn't justification for allowing strings. I don't know how or why that casting got in there, but I imagine it was put in merely as a fallback measure. A measure that is seemingly redundant and ultimately masks when a module isn't providing the correct data: an array.
In fact, the whole point of receiving an error like this is to inform the developer that there is bad code. Casting something like this isn't a solution, it's simply ignoring the problem and perpetuating bad code/practice.
I'm not willing to add in code that ultimate encourages bad coding by developers who choose to ignore "Best Practices™".
Regardless, this project made the decision to only support class arrays a long time ago (see related issue and #326539: Convert 'class' attribute to use an array, not a string).
Comment #3
dpiThanks, was fishing to see if you would match core behaviour. But I totally understand why you wouldnt support it.
I only ask because I can see quite a few instances in core and contrib where this still occurs. There is even core test for it (
AttributeTest::testConstructor
):core
block.module: blocklistbuilder
comment.module: commentlinkbuilder
config_translation.module: ConfigTranslationBlockListBuilder::buildRow
ckeditor.module
....
contrib
calendar.module
ctools
crm_core
devel
ds
...
Comment #4
markhalliwellFWIW, I just remembered why core allows this: it's because Twig uses the same PHP class to handle it's attribute modifications. In twig, one needs to be able to add either a string or an array of classes. No where in core (aside from tests) are there classes passed as strings in the render array. Since this helper class/method only relates to render arrays in PHP, not Twig, there's no need to support strings. This is just another symptom of how broken the render array "API" is since it technically allows any arbitrary value and there's no "check" until something attempts to consume it.
All PHP string instances of render array classes need to be squashed from contrib existence. There's no real justification for why a developer cannot use an array. Yes, before it was argued that it was bulky/lengthy:
array('some-class')
. However, now one can simply do:['some-class']
.