#2486267: Attributes of a block content are applied to block itself broke what might be a fairly common workaround for the fact that attributes were pulled up to the top-level block template, putting your actual element into a child key and keeping attributes on top-level without anything else:
From an example we use in a project:
$build = [
'#attached' => ['library' => ['mymodule/something']],
'#attributes' => ['class' => ['wrapper-class'],
'link' => [
'#theme' => 'mymodule_something',
'#label' => $config['text'],
'#attributes' => [
'href' => '#main-content',
'class' => $link_classes,
'aria-label' => $config['text'],
] + $data,
],
];
This works fine up until 11.3 but the top-level attributes completely vanish in 11.4.
I think we can continue to pull up attributes for BC *if* $content has neither type or theme set.
Show commandsStart 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:
changes
Comments
Comment #3
berdirCreated a MR for my proposal in #2486267: Attributes of a block content are applied to block itself
Comment #4
smustgrave commentedShould this have test coverage?
Comment #6
godotislateConsulted with @catch, and this looks like something that'd be good to get in for 11.4.
Comment #7
berdir@godotislate: do either of you have an opinion on whether not this should trigger a deprecation message? It's minimal extra code, but you could argue that having two ways to do something can be confusing. My suggestion would be to commit the hotfix and if we want to consider deprecating that, do it in a follow-up.
Test coverage looks pretty good to me, wasn't fully aware that #children works to to reproduce this, but makes sense. Would be nicer to use actual CSS asserts to verify on which element the extra classes and attributes are instead of just somewhere, but not sure how many options we have for that in kernel tests.
Comment #8
godotislateI haven't look much at the original issue, but IIUC there's no deprecation warning or any indication that changes may need to be made, other than if a project has CI tests running tests against rendered block mark up or visual regression? If so, I think some kind of deprecation message might make sense if possible, but yes, to me that's OK for follow up.
Speaking of
#children,Drupal\block_test\Plugin\Block\TestHtmlBlockalready exists, so maybe we don't need to introduce a new test block plugin?I wonder if the rendered block can be loaded as a DOM object via Html::load(), then DOM querying can work against that?
Haven't looked at the MR fully here yet otherwise.
Comment #9
berdir> I haven't look much at the original issue, but IIUC there's no deprecation warning or any indication that changes may need to be made, other than if a project has CI tests running tests against rendered block mark up or visual regression? If so, I think some kind of deprecation message might make sense if possible, but yes, to me that's OK for follow up.
No, there's nothing like that and it's also an immediate behavior change/fix (depends on who you ask I guess) in a minor.
> Speaking of #children, Drupal\block_test\Plugin\Block\TestHtmlBlock already exists, so maybe we don't need to introduce a new test block plugin?
Yes, that's what the test does, that's why I realized that #children also works like that.
We also in fact used to have test coverage of the current behavior using that feature, in \Drupal\Tests\block\Functional\BlockHtmlTest, and the previous issue had to fix that, that's why support for wrapper_attributes was added to that block. That's a functional test doing a real request and that can use $this->assertSession()->elementExists().
We could just restore that test to use #attributes again, because with this change, that still works like that. But then wouldn't have test coverage for wrapper_attributes, can't see any other usage of that anywhere.
Comment #10
berdir> Yes, that's what the test does, that's why I realized that #children also works like that.
Sorry, that was wrong, I misread that. I have now simplified it and used the existing block, I think that's sufficient and I verified it fails without the fix.
Comment #11
godotislateI think this looks good. We should probably add to CR https://www.drupal.org/node/3525287 for #2486267: Attributes of a block content are applied to block itself if this gets merged to 11.4.
Comment #12
catchYes agreed with this, when I looked at the MR, I wasn't 100% sure about the 'BC' in the comment, but in a follow-up we can decide whether to deprecate it or leave it as a permanent semi-feature.
Comment #13
berdirI created #3605585: Consider formally deprecating pulling up attributes in block plugins without type/theme as a follow-up to discuss BC. I also have a proposal on how to structure and update the CR from the original issue over there because the current CR is very confusing and at least partially incorrect.