Drupal\Core\Template\Attribute doesn't support adding attributes with array syntax if attribute name not already initialised

Created on 17 January 2023, almost 2 years ago
Updated 30 August 2024, 4 months ago

Problem/Motivation

The docs for Drupal\Core\Template\Attribute say

> add attributes using array syntax

and give this example:

 *  $attributes = new Attribute(array('id' => 'socks'));
 *  $attributes['class'] = array('black-cat', 'white-cat');
 *  $attributes['class'][] = 'black-white-cat';

However, if you try to use this as an array without initialising a property, like this:

 *  $attributes = new Attribute(array('id' => 'socks'));
 *  $attributes['class'][] = 'black-white-cat';

you get this error:

> Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect

Steps to reproduce

Proposed resolution

Make it not produce an error.

Remaining tasks

User interface changes

N/A

API changes

None.

Data model changes

None.

Release notes snippet

๐Ÿ› Bug report
Status

Downport

Version

10.4 โœจ

Component
Themeย  โ†’

Last updated 33 minutes ago

Created by

๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • I tried to reproduce what's mentioned in the issue but i was not able to do that.I will write some steps that i took trying to reproduce the issue.
    Steps to reproduce:-
    1) Install the ajax form test module.
    2) go to /ajax_forms_test_get_form url.
    3) In the buildform method I modified the following select element as below:-

    $form['select'] = [
          '#title' => $this->t('Color'),
          '#type' => 'select',
          '#options' => [
            'red' => 'red',
            'green' => 'green',
            'blue' => 'blue',
          ],
          '#attributes' => $attributes->toArray(),
          '#ajax' => [
            'callback' => [$object, 'selectCallback'],
          ],
          '#suffix' => '<div id="ajax_selected_color">No color yet selected</div>',
        ];
        $form['select']['#attributes']['class'][] = 'TestingAttributes';

    With the above changes applied I should get the error mentioned above, but i was able to get the class rendered on the page.
    Attaching the SS for the same.

    I also tried the same steps on system.admin.inc file and modified the way attributes is added on line 285 and 294 but there also i was able to get the class rendered.

    It would be great if someone who can reproduce this, add some steps to reproduce in the issue summary.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    The steps to reproduce are already in the issue summary.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune

    yash.rode โ†’ made their first commit to this issueโ€™s fork.

  • Merge request !8310added test and fix โ†’ (Closed) created by yash.rode
  • Status changed to Needs work 7 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    Thanks for working on this!

    Tests are failing however.

    Also, I don't think it's a good idea to put a special case for a particular data provider in a data-provided test method. Better to add a dedicated test method for this case. It's a unit test anyway, so it's fast.

  • Pipeline finished with Failed
    7 months ago
    Total: 1606s
    #192498
  • Pipeline finished with Success
    7 months ago
    Total: 546s
    #192530
  • Pipeline finished with Success
    7 months ago
    #192550
  • Status changed to Needs review 7 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia yash.rode pune

    This is ready for another round of review.

  • Pipeline finished with Canceled
    7 months ago
    Total: 384s
    #192621
  • Pipeline finished with Success
    7 months ago
    Total: 520s
    #192629
  • Pipeline finished with Success
    7 months ago
    Total: 540s
    #192667
  • Pipeline finished with Success
    7 months ago
    Total: 541s
    #193458
  • Status changed to Needs work 7 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Sorry can the issue summary be updated to include a proposed solution. Not sure if this is an api change or not (didn't look deeply).

    Thanks.

  • Status changed to Needs review 7 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    This isn't an API change, it's fixing a PHP error you get if you try to use Attribute with the array access as documented.

    The proposed solution is 'make it work properly'.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Not sure that's a proper summary lol so will just leave here for the next reviewer. Seen enough issues kicked back for not having proper issue summary.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    I'll remove the 'TBD' because that doesn't look good, but honestly I don't see that there's anything relevant to put there.

    Doing $attributes['class'][] = 'black-white-cat'; produces an error. Make it not produce an error.

  • Status changed to Needs work 6 months ago
  • The Needs Review Queue Bot โ†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide โ†’ to find step-by-step guides for working with issues.

  • Status changed to Needs review 6 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    Rebased.

  • Pipeline finished with Success
    6 months ago
    Total: 754s
    #210875
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim
  • Status changed to RTBC 5 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Believe feedback on this one has been addressed.

  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡ณ๐Ÿ‡ฟNew Zealand quietone

    Left some comments in the MR asking for more comments and some simplification.

  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom joachim

    I think I've addressed everything -- the MR is not letting me mark things are resolved?!?!

  • Pipeline finished with Canceled
    4 months ago
    Total: 240s
    #253701
  • Pipeline finished with Failed
    4 months ago
    Total: 189s
    #253703
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Appears to have a cspell error

    And only the original author of the MR can close threads. Which is a bummer

  • Pipeline finished with Success
    4 months ago
    Total: 759s
    #253876
  • Status changed to RTBC 4 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Believe feedback has been addressed.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom catch

    Committed/pushed to 11.x and cherry-picked to 11.0.x. I think this is worth backporting to 10.4.x but it doesn't cherry-pick cleanly so moving to 'to be ported'.

    • catch โ†’ committed b7553795 on 11.0.x
      Issue #3334045 by yash.rode, joachim, quietone: Drupal\Core\Template\...
    • catch โ†’ committed ca19c94c on 11.x
      Issue #3334045 by yash.rode, joachim, quietone: Drupal\Core\Template\...
  • Status changed to Downport 4 months ago
Production build 0.71.5 2024