toolbar_preprocess_html() converts attributes from array to Attribute object

Updated the issue summary to expand on the issue at hand.

The patch in #2 solves the fatal error in our custom theme implementing hook_preprocess_html() after updating to Drupal 8.4.0

Well now we're in the worst of all worlds because there might code out there assuming this is an attribute :(

@idebr since Attribute implements ArrayAccess it'd be great if this issue detailed what your custom theme is doing in hook_preprocess_html()?

Re #8

Well now we're in the worst of all worlds because there might code out there assuming this is an attribute :(

Correct, currently $variables['attributes'] in hook_preprocess_html() may be an array or Attribute depending on where the hook is implemented. Assuming a stock Drupal installation where hooks are called in order of modules, themes and then by alphabet:

1. For module implementations where the hook starts with an a[...] through toolbar[...], $variables['attributes'] is an array
2. For module implementations after toolbar and themes, $variables['attributes'] is an Attribute

@idebr since Attribute implements ArrayAccess it'd be great if this issue detailed what your custom theme is doing in hook_preprocess_html()?

The project consists of a multisite installation with a custom theme for the corporate site with a subtheme for its intranet. The intranet theme is stripped from flashy content, since its main focus is to provide textual content.

The following code:

/**
 * Implements hook_preprocess_HOOK().
 */
function intranet_preprocess_html(&$variables) {
  $swoosh_class_index = isset($variables['attributes']['class']) ? array_search('page--with-swoosh-photo', $variables['attributes']['class']) : FALSE;
  if ($swoosh_class_index !== FALSE) {
    unset($variables['attributes']['class'][$swoosh_class_index]);
  }
}

resulted in the following notice:

Warning: array_search() expects parameter 2 to be array, object given in intranet_preprocess_html() (line 95 of themes/custom/intranet/intranet.theme).

@alexpott Did the additional information help at all? Tentatively updating status back to RTBC, since coding around switching data types makes coding in this area prone to errors.

Thank you @bojan_dev for opening the issue and creating the patch, and thank you @idebr for a clear explanation of the problem. Sorry for both of you that it has taken so long to provide feedback on this issue.

I discussed this issue today with @alexpott. We agreed that there are two possible ways to proceed with this:

1. Document a known workaround for the bug and make a proper fix in the next major release. If we leave the code as is, themes and modules are prone to errors when enabling or disabling toolbar module. Reason to consider this option is that it wouldn't at least immediately break any sites.
2. Fix the issue as proposed on this issue by removing the array to Attribute object conversion. This is a BC break which should be documented in a change record. Any modules or themes that assume that the attributes array item type is Attribute are affected. This change could also affect any modules or themes that have worked around this bug already.

According to our Drupal 8 backwards compatibility and internal API policy, preprocess function values can be changed. Given that the policy would allow us to make the code change, and the fact that the bug fix is reverting a relatively recent change, I'm leaning towards suggesting to proceed with the option 2.

I added a draft change record for #11.2 at https://www.drupal.org/node/3002434

Closed #3015783: type of $variables['attributes'] in hook_preprocess_html as a duplicate.

I updated the issue summary with more information.

Sadly I still cannot think of a way to fix this without any disruption. However, In my opinion, we cannot leave the code as it is. The fact that Toolbar module changes the type of the 'attribute' variable into Attribute object makes theming fragile because some themes might incorrectly expect the variable to be Attribute object, which it isn't consistently since it depends on whether Toolbar module is enabled or not. Some themes might have already built code that relies on 'attributes' being Attribute object, but in fact, that code is broken at the moment since it creates dependency on Toolbar module being enabled.

Hopefully, in future, we will run some type checks for variables: #3016948: Type check theme variables, but in the meanwhile, we just have to be more careful to not introduce this kind of changes.

Given the Drupal Core frontend BC policy, I think option #11.2 is sufficient. Any decent theme should be prepared to handle either an array or Attribute object; that's what the Drupal Bootstrap base theme does.

The CR is a little muddy though...

For a consistent developer experience, the 'attributes' variable in html.html.twig is now consistently an array.

This seems to imply that the Twig template variable would suddenly become an array. This isn't true as ThemeManager::render ensures that 'attributes', 'title_attributes', 'content_attributes' all become Attribute objects prior to Twig template rendering.

Thank you for the review @markcarver! I agree that the change record could use some more clarity, so I went ahead and updated it. Feel free to take another look at that.

CR looks good to me.

Was planning on marking this as RTBC, but it seems there still needs to be a release manager review?

Also, seems a bit unnecessary to have this be an array + foreach loop; they can just be simple additions... here's a new patch (no interdiff, patch is extremely tiny).

+1 on the new patch.

Looks we have a concession here that #11.2 is the way to go (I agree too), lets see if a release manager agrees :)

Seems like a random test failure.

Another random test failure...

Testbot:
15 Feb 2019 at 13:19 CET: PHP 7 & MySQL 5.5 Build Successful

Sadly, the testbot got all kinds of confused yesterday. I've had to re-upload patches to a number of issues to unstick weirdness. Agreed #18 is RTBC. Here it is again. This should get the testbot to stop marking this needs work.

Agreed on changing this back, at the moment it depends when your hook is fired and whether toolbar is enabled, so could affect any new site seemingly at random. With the patch applied, we have consistent behaviour that won't affect new sites and would only affect a subsection of existing sites that have adapted to the bug since 8.4...

Committed c5b88d8 and pushed to 8.7.x. Thanks!

We can haz backport? ;) Maybe too disruptive in a patch release. But I don't see any explicit mention that this had to wait for a minor release, and @lauriii participated here multiple times. I would have assumed they would have said so if this wasn't backportable. ;) The issue lived on the 8.6.x branch for quite a while, and that's what we were testing against.

Thoughts?

Thanks,
-Derek

p.s. CR needs updates to clarify the version(s) where this is fixed, and to be published.

Sad, sad confused bot. :/ Here's #18/#27 yet again. A raw git diff this time, without the commit metadata.

I think this bug fix is too risky to be included in a patch release since some modules and themes built after 8.4.0 might have to be updated. I updated the CR and published it!

Okay, sounds good. Thanks for being explicit (and fixing/publishing the CR!).

Cheers,
-Derek