Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
It turns out that when you render Seven's entity-add-list.html.twig template, you cannot set additional attributes on the bundle names, because Seven does this in the template:
{% for bundle in bundles %}
<li class="clearfix"><a href="{{ bundle.add_link.url }}"><span class="label">{{ bundle.label }}</span><div class="description">{{ bundle.description }}</div></a></li>
{% endfor %}
The problem is Seven unpacking the bundle.add_link
variable, which *should* be unnecessary because it implements RenderableInterface. But anyhoo, because it unpacks the variable, any attributes set on the link's URL object are thrown right out the window. We need something like this:
{% for bundle in bundles %}
<li class="clearfix"><a{{ create_attribute(bundle.add_link.url.options.attributes) }} href="{{ bundle.add_link.url }}"><span class="label">{{ bundle.label }}</span><div class="description">{{ bundle.description }}</div></a></li>
{% endfor %}
Special thanks to Wim Leers for heroically tracing this and suggesting the fix.
Comment | File | Size | Author |
---|---|---|---|
#46 | interdiff_44-46.txt | 526 bytes | raman.b |
#46 | 2836381-46.patch | 5.06 KB | raman.b |
#44 | 2836381-44.patch | 5.03 KB | Marcin Maruszewski |
#31 | 2836381-13.patch | 5.02 KB | Wim Leers |
#31 | 2836381-13-FAIL.patch | 4.18 KB | Wim Leers |
Comments
Comment #2
phenaproximaHere's the patch.
Comment #3
Wim LeersI think this is ready, but I'm the one who suggested that exact fix, so I shouldn't be the one to RTBC this.
This is blocking #2831274: Bring Media entity module to core as Media module.
Comment #4
RainbowArrayLooks solid to me. I've been a bit absent from the queues, so create_attribute is new to me. I looked at the documentation, though, and this makes sense. Essentially passing the options.attributes from the add_link and manually placing them on the a element. Seems reasonable.
Comment #5
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedRTBC + 1
Comment #6
phenaproximaI have a sense that we are missing something here. Should we be putting bundle.add_link.url.options.attributes in the template, or just bundle.add_link.options.attributes, which is what the code in \Drupal\Core\Render\Element\Link::preRenderLink() would seem to indicate?
I think an experienced themer needs to look at this before we can go to RTBC. Sorry, guys...
Comment #7
seanBThe template receives the link object directly and unpacks it so \Drupal\Core\Render\Element\Link::preRenderLink() is not used in this case.
For now I think it's RTBC.
When checking this, I saw the node add list and block content add list pages look the same but are built and rendered differently. It's currently not possible to add options for those links (the preprocessors are not providing options). It would be nice to make the developer/themer experience for this similar, but that's probably best discussed in a follow up.
Comment #8
alexpottAt the very least this needs tests but also the divergence between node-add-list and this and also this and the other themes is a concern.
Comment #9
phenaproximaI can try to come up with tests, but regarding the divergence of this and node-add-list...well, I can't speak to why that happened, but my suspicion (based on no real evidence) is that the whole idea of a generic bundle list was in Node first, then cross-ported into the core entity system, and subsequently never removed from Node. I think they should be merged, but that would be a separate issue.
Comment #10
phenaproximaBehold, a test. It's an awful lot of jury-rigging for a one-line fix, but hey...you gotta do what you gotta do.
I'm sorry about the use of the Standard profile; I couldn't figure out how to install Seven and get it to be the default admin-side theme with the Testing profile. If anyone can point me in the proper direction there, I'd appreciate it. Until then, this should do.
Comment #13
phenaproximaDrupal CI is such an unforgiving mistress. Forget one little @group annotation and suddenly you're a damn pariah with a bad patch.
Comment #15
seanBPatch looks good. Even a FAIL patch to prove it. Nice work!
Comment #18
alexpottCommitted and pushed f01ac2d to 8.3.x and f5c7961 to 8.2.x. Thanks!
I've committed this to 8.2.x because seven is internal and the default entity-add-list.html.twig all just print
bundle.add_link
so will have the attributes.Comment #21
BerdirHm, I had a test fail on this in https://www.drupal.org/pift-ci-job/557210. Not reproducible.
Comment #22
drugan CreditAttribution: drugan as a volunteer commentedConfirm the test fail related to this commit:
https://www.drupal.org/pift-ci-job/557855
Can't figure out where changes on removing test site directory might intersect with displaying link on a particular theme.
Comment #24
alexpottYep this introduced a random fail :(
#2836237-11: Views with a different query plugin created via the UI do not have the correct query plugin ID in the view config and https://www.drupal.org/pift-ci-job/557961
I guess the random ness in the test - it'd be good to work out why.
Comment #29
chr.fritschI ran this test 150 times on my local machine and got zero test fails. Anyone an idea what to do here?
Comment #30
andypostLooks something in core omits to initialize attributes for url somehow
Comment #31
Wim LeersI can't imagine this is causing a random fail. I wonder if the random fail was merely being exhibited through this change, but the root cause was elsewhere?
#29 makes me believe that even more.
#13 still applies cleanly. Reuploading.
Comment #33
phenaproximaRe-tagging.
Comment #34
phenaproximaHonestly, I don't see how this could possibly have introduced a random failure either. I mean...maybe an errant functional JavaScript test that's not waiting for something it needs? But if that's the case, I have a hard time imagining that this little change to Seven would have broken that.
I would mark this for subsystem maintainer review, but...Seven has no listed maintainers in MAINTAINERS.txt. So I'm just going to kick it back to RTBC.
Comment #35
xjmThere was definitely a random fail introduced by this issue in December. Being dismissive and claiming there was no random fail introduced by the patch is not helpful, because there was, and it started failing immediately after commit: https://www.drupal.org/pift-ci-job/557210. Whether or not the underlying cause was elsewhere, we can't have a test that randomly fails in core, because that's introducing a critical regression.
In order to commit it, we need to prove the random fail is no longer present, and in order to do that, we would need to locate the cause. Research into the fail needs to be documented on the issue.
Thanks everyone!
Comment #36
xjmSee #21, #22, #24, and #30.
Thanks @chr.fritsch for running the test locally. That's a good first step. I recommend also running it more times locally, like 1000. (150 runs only has a ~3 in 4 chance of surfacing a 1% fail.) This can be done on the commit hash for the original commit.
The next step is to see if it's something that fails on the bot but not locally, which is often the case. We can create a test-only patch that runs only this test several hundred times, in a test issue, adn then trigger multiple runs of that to get a baseline for the fail rate now.
To get a baseline fail rate on the bot for when this was committed, we could maybe use 8.2.x to test it (since it's unfeasible to post a patch that reverts the entirety of 7 months of code) or even 8.1.x, but let's start with those other steps.
Comment #38
vijaycs85Patch in #31 still applies cleanly on 8.5.x and tests are green locally:
Comment #44
Marcin Maruszewski CreditAttribution: Marcin Maruszewski at RatioWeb commentedI noticed that on default add bundle pages, where there are no extra attributes (the Url object options attribut is empty array, so options.attributes parameter is a NULL) there is an error like this
TypeError: Argument 1 passed to Drupal\Core\Template\TwigExtension::createAttribute() must be of the type array, null given
It means, that we cannot pass NULL to the createAttribute() method. It should be an array, even empty. I suggest add simple `Null coalescing operator` so we will pass in create_attrbute function eather initialized options attributes array from Url object options, or an empty array if the mentioned value is a NULL.
Comment #46
raman.b CreditAttribution: raman.b at OpenSense Labs commentedComment #47
tanubansal CreditAttribution: tanubansal at Salsa Digital commentedTested #46 on 9.1
RTBC + 1
Comment #52
longwaveThe Seven theme has been removed from Drupal 10 core. I confirmed that this issue only affects Seven and no other themes included with Drupal core, so I am moving this to the contributed Seven project.