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.
CommentFileSizeAuthor
#9 write_test_to_ensure-2420967-9.patch729 bytesJeroenT
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JeroenT’s picture

.

JeroenT’s picture

Issue summary: View changes
idebr’s picture

I could only find this @todo in LinkGeneratorTest:

@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.

Is this the @todo you are referring to or is there a different one?

JeroenT’s picture

Issue summary: View changes

@idebr,

Yes that is the todo. I added the already altered one from #2388627: Remove usages of drupal_is_front_page.

JeroenT’s picture

Issue summary: View changes
neclimdul’s picture

So 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.

Wim Leers’s picture

Status: Active » Needs work

#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 comprehensive?) 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 :)

Wim Leers’s picture

Status: Needs work » Active
Issue tags: -Needs tests

Oops.

JeroenT’s picture

Status: Active » Needs review
FileSize
729 bytes

Added patch that removes the todo.

Wim Leers’s picture

Title: Write test to ensure that the active class is added on links when on the front page. » Write test to ensure that the active class is added on links when on the front page
Priority: Normal » Minor
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quickfix
neclimdul’s picture

Component: base system » documentation

Documentation fix as wel. +1 RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay, fewer @todos. :)

Committed and pushed to 8.0.x. Thanks!

  • webchick committed a2ef3c3 on 8.0.x
    Issue #2420967 by JeroenT, Wim Leers, neclimdul: Write test to ensure...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.