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.Problem/Motivation
The \Drupal\Core\Menu\ContextualLinkManager::getContextualLinksArrayByGroup() calls the ContextualLinkInterface::getTitle() implementations with an argument (which is a request object) but the interface definition does not suggests that an argument will be passed always.
Instead the manager is supposed to use the controller resolver as the \Drupal\Core\Menu\LocalActionManager does.
The docs for \Drupal\Core\Menu\LocalActionInterface::getTitle and \Drupal\Core\Menu\ContextualLinkInterface::getTitle are nearly identical and indicate the controller resolver will be used.
Proposed resolution
Call ContextualLinkInterface::getTitle via the controller resolver (if possible) or fix the docs to indicate the request object is the only possible parameter.
Allowed?
https://www.drupal.org/core/d8-allowed-changes
Might be allowed in 8.2.x
https://www.drupal.org/core/d8-allowed-changes#beta
as a non-disruptive bug fix
https://www.drupal.org/core/release-cycle-overview#dates
8.2.x is in beta until the Sept 7 rc1
Remaining tasks
- (done) create followup. #2796153: Refactor ContextualLinkInterface LocalActionInterface LocalTaskInterface MenuLinkInterface to use a shared Interface to reduce code duplication
User interface changes
No
API changes
Probably none - adding better docs and test coverage
Data model changes
No
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 2783375-9.patch | 3.48 KB | YesCT |
| #6 | 2783375-6.patch | 2.19 KB | YesCT |
| #6 | interdiff.2783375.4.6.txt | 1.84 KB | YesCT |
| #4 | 2783375-4.patch | 732 bytes | Wim Leers |
Comments
Comment #2
SweetchuckComment #3
YesCT CreditAttribution: YesCT commentedSo, one thing we can do is look for usages of getTitle() and if any that pass an arg, all of those do not *use* the arg, then we could take the arg out and leave the interface as it is. So I guess we are searching for some usage where the arg *is* used.
Comment #4
Wim LeersNice catch!
AFAICT that's just a c/p remnant. Nothing relies on it. Let's just remove it.
Comment #5
YesCT CreditAttribution: YesCT commentedI think we need to change some more things (where it was called with a request). I'm working on a new patch.
Comment #6
YesCT CreditAttribution: YesCT commentedI went through
ag "\->getTitle.*request" coreAnd looked for any that were using $request that should not.
Lots were from TitleResolverInterface getTitle() and seemed fine.
I did *not* figure out core/modules/system/tests/modules/batch_test/batch_test.module call to getTitle()
And
LocalActionDefaultTest::testGetTitleWithTitleArguments() looks related but different because it is
LocalActionDefault which has a getTitle() with a request arg, but it does not use the arg, and it is an
LocalActionInterface which has no arguments to getTitle()
#2795235: Inaccurate getTitle() signature in LocalActionDefault for that.
---
these changes are
1. to remove the use that is not needed when the request arg is taken out,
2. to take out the request (which was not used) from the methods that were adding it as an arg
---
lets see if this causes any tests to fail.
---
not minor according to https://www.drupal.org/core/issue-priority#minor
Comment #8
YesCT CreditAttribution: YesCT commentedI'm looking into the fails. Test results point to UserTokenReplaceTest
running locally with
php core/scripts/run-tests.sh --sqlite /tmp/drupal/test.sqlite --dburl mysql://root:root@localhost/drupal2 --url http://localhost:8888/drupal2 --class "Drupal\user\Tests\UserTokenReplaceTest"Drupal\user\Tests\UserTokenReplaceTest 60 passes
hm. gonna retest.
Comment #9
YesCT CreditAttribution: YesCT commentedcombining with #2795235: Inaccurate getTitle() signature in LocalActionDefault per #11 there.
bit worrying that that also had some fails
https://www.drupal.org/pift-ci-job/449085
Comment #10
YesCT CreditAttribution: YesCT commentedFollow up created:
#2796153: Refactor ContextualLinkInterface LocalActionInterface LocalTaskInterface MenuLinkInterface to use a shared Interface to reduce code duplication
Comment #11
pwolanin CreditAttribution: pwolanin as a volunteer commentedI'm pretty sure the Request object should be passed there based on the code in places like the ContextualLinkManager
You need the request or something to e.g. resolve the current user.
Comment #12
pwolanin CreditAttribution: pwolanin as a volunteer commentedThe LocalTaskManager does:
So... this is like the way Symfony allows you to add the request as an argument to any route controller. Maybe it's just a documentation gap?
Comment #13
pwolanin CreditAttribution: pwolanin as a volunteer commentedThis is one of those screwy things I recall discussing with maybe dawehner or tim.plunkett
When you are using the controller resolver, the interface has no args, but any implementation can ask for args with a default of NULL.
So, I'm not sure this issue is going in the right direction.
The discrepancy between contextual link and the others seems like the problem in addition to weak docs.
LocalActionManager also uses the controller resolver
MenuLink is expected to be much more flexible behind the plugin facade, so it's not using the same approach at all
Comment #14
pwolanin CreditAttribution: pwolanin as a volunteer commentedComment #23
larowlanAgree with #13