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.
Todo in LinkGeneratorTest class:
* @todo Test that the active class is added on the front page when generating
* links to the front page when drupal_is_front_page() is converted to a
* service.
Comment | File | Size | Author |
---|---|---|---|
#9 | write_test_to_ensure-2420967-9.patch | 729 bytes | JeroenT |
Comments
Comment #1
JeroenT.
Comment #2
JeroenTComment #3
idebr CreditAttribution: idebr commentedI could only find this @todo in LinkGeneratorTest:
Is this the @todo you are referring to or is there a different one?
Comment #4
JeroenT@idebr,
Yes that is the todo. I added the already altered one from #2388627: Remove usages of drupal_is_front_page.
Comment #5
JeroenTComment #6
neclimdulSo did some archaeology around this. Back in the day, l() autodetected the active class on the link. So it came that when l was converted into a service and this test was created this todo was added. But then along came #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links and everything changed.
Long story short I don't think this responsibility lives in the LinkGenerator any more and the @todo just needs to be deleted.
Comment #7
Wim Leers#6 is totally correct! :)
It's now done on the client-side, in JS, for authenticated users. And for anonymous users, it's done in PHP (we assume pages for anonymous users are cached using either the built-in Page Cache, or some other reverse proxy like Varnish).
We don't have a JS testing infrastructure and hence no tests (#sadpanda), but fortunately it's simple. See
core/misc/active-link.js
.We do have very, very complete (dare I say
?) test coverage for the PHP alternative — see\Drupal\Tests\system\Unit\Controller\SystemControllerTest::testSetLinkActiveClass()
, there are literally thousands of test cases there!Therefore this issue can simply delete that
@todo
:)Comment #8
Wim LeersOops.
Comment #9
JeroenTAdded patch that removes the todo.
Comment #10
Wim LeersComment #11
neclimdulDocumentation fix as wel. +1 RTBC.
Comment #12
webchickYay, fewer @todos. :)
Committed and pushed to 8.0.x. Thanks!