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

dpi created an issue. See original summary.

markhalliwell’s picture

Version: 8.x-4.x-dev » 8.x-3.x-dev
Component: Code » Miscellaneous
Category: Bug report » Support request
Status: Active » Closed (works as designed)
Related issues: +#2269653: [bootstrap][policy][7.x-3.x] CSS Classes (Fatal error: [] operator not supported for strings)

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

dpi’s picture

Thanks, 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

...

markhalliwell’s picture

I only ask because I can see quite a few instances in core and contrib where this still occurs.

FWIW, 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'].