Problem/Motivation

Following #3515506: Process #attributes render property for SDCs, testing 11.3-dev with a few SDC themes and projects, I have met TypeError: Drupal\Core\Template\Attribute::merge(): Argument #1 ($collection) must be of type Drupal\Core\Template\Attribute, array given when there is already a SDC attribute prop with an array value.

Steps to reproduce

Any component renderable with an array as attributes prop:

[
  '#type' => 'component',
  '#component' => 'foo:bar',
  '#attributes' => [
    'foo' => 'bar',
  ],
  '#props' => [
    'attributes' => [
      'lorem' => 'ipsum',
    ]
  ]
]

Proposed resolution

ComponentElement::mergeElementAttributesToPropAttributes() is already aware about this issue:

// If attributes value is an array, convert it to an Attribute object as
// \Drupal\Core\Template\Attribute::merge() expects an Attribute object.

But it is converting the other value:
$element_attributes = is_array($element['#attributes']) ? new Attribute($element['#attributes']) : $element['#attributes'];

So, let's keep this conversion and add the conversion of the value which must fit \Drupal\Core\Template\Attribute::merge() expectations.

API changes

We don't break anything. But we may need a new test.

Issue fork drupal-3550051

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

pdureau created an issue. See original summary.

pdureau’s picture

pdureau commented
Version: 11.2.x-dev » 11.x-dev

pdureau changed the visibility of the branch 3550051-convert-attributes-props to hidden.

pdureau opened merge request !13394

pdureau’s picture

pdureau commented
Status: Active » Needs work

MR opened, still iterating.

christian.wiedemann’s picture

christian.wiedemann commented

Looks perfect.

christian.wiedemann’s picture

christian.wiedemann commented
Status: Needs work » Reviewed & tested by the community
christian.wiedemann’s picture

christian.wiedemann commented

Only one nitpic which is already wrong in the current MR
If Attributes is filled with "something" which is not an array and not attributes a type check would be good. (But this is also the case for attributes itself)
So a type check would increase the security:

  $raw_prop_attributes = $element['#props']['attributes'];
  $prop_attributes = $raw_prop_attributes instanceof Attribute
    ? $raw_prop_attributes
    : (is_array($raw_prop_attributes) ? new Attribute($raw_prop_attributes) : new Attribute());

  $element['#props']['attributes'] = $element_attributes->merge($prop_attributes);
christian.wiedemann’s picture

christian.wiedemann commented
Status: Reviewed & tested by the community » Needs work
pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned
Status: Needs work » Needs review
Issue tags: +11.3.0 release priority

Thanks Christian for your suggestion. It makes sense but I would prefer to keep the MR focused on the fatal error we are already observing in the wild.

astonvictor’s picture

astonvictor commented
Status: Needs review » Closed (duplicate)

It's already fixed by #3515506: Process #attributes render property for SDCs

Here are changes that were committed - https://git.drupalcode.org/project/drupal/-/commit/103108a6668662adf5fe7...

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

astonvictor’s picture

astonvictor commented
Related issues: +#3550051: Convert attributes props to an Attribute object before Attribute::merge()
pdureau’s picture

pdureau commented
Status: Closed (duplicate) » Needs review

Hi Viktor,

As you can read in the issue description, this is not a duplicate of #3515506: Process #attributes render property for SDCs but a follow-up.

I am pretty confident about this because I am partially responsible of introducing this bug in #3515506: Process #attributes render property for SDCs where we check one of the two attributes data ($element['#attributes']) without checking the other ($element['#props']['attributes']) 😳

pdureau’s picture

pdureau commented

To also credit: @schillerm which has helped in the contribution room in Vienna.

schillerm’s picture

schillerm commented

Hi, yes I reproduced the error on 11.x and reviewed and tested !MR13394. Works as expected for me. +1 for RTBC
Had to learn more about working with core so that's good.

schillerm’s picture

schillerm commented
Status: Needs review » Reviewed & tested by the community
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Fix looks straight forward - let's get a test here - the steps to reproduce seem pretty simple

Thanks!

pdureau’s picture

pdureau commented
Assigned: Unassigned » pdureau

OK, I will add or extend a test case in core/tests/Drupal/KernelTests/Components/ComponentRenderTest.php

It will also be the opportunity to complete a missing part of the logic:

  • Prop: no. Element: no. ✅ already tested
  • Prop: no. Element: object. ✅ already tested
  • Prop: no. Element: array. ✅ already tested
  • Prop: object. Element: no. ✅ already tested
  • Prop: object. Element: object. ✅ already tested
  • Prop: object. Element: array. ✅ already tested
  • Prop: array. Element: no. ❌ test to add, and logic to fix
  • Prop: array. Element: object. ⚠️ test to add
  • Prop: array. Element: array. ⚠️ test to add
pdureau’s picture

pdureau commented
Assigned: pdureau » larowlan
Status: Needs work » Needs review

Hi @larowlan: Test added. Is it OK?

christian.wiedemann’s picture

christian.wiedemann commented

Looks good now

christian.wiedemann’s picture

christian.wiedemann commented
Status: Needs review » Reviewed & tested by the community
pdureau’s picture

pdureau commented
Issue tags: -Needs tests

Thanks Christian.

The last KO in pipleine is a functional test: testNoLayoutSave with data set "revert":

   Inline Block (Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlock)
     ✔ Inline blocks
     ✔ No layout save with discard_changes
     ✘ No layout save with revert
       ┐
       ├ TypeError: str_starts_with(): Argument #1 ($haystack) must be of type string, array given
       │
       │ /builds/issue/drupal-3550051/core/tests/Drupal/FunctionalJavascriptTests/WebDriverTestBase.php:115

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/layou...

There is no manipulation of SDC here, only User Session and Entity Storage. So I don't beleive it is related

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Assigned: larowlan » Unassigned
Status: Reviewed & tested by the community » Fixed

Committed and pushed 97116f5319d to 11.x and a64e7d785fb to 11.3.x. Thanks!

Backported to 11.3.x as a priority because this bug was introduced there.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed a64e7d78 on 11.3.x 
    Issue #3550051 by pdureau, christian.wiedemann, schillerm, larowlan:...

  • alexpott committed 97116f53 on 11.x 
    Issue #3550051 by pdureau, christian.wiedemann, schillerm, larowlan:...

Status: Fixed » Closed (fixed)

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