Comment permalinks are cached incorrectly when multiple pages exist

Thanks for the detailed report

Code to look at here is \Drupal\comment\Controller\CommentController::commentPermalink

Test coverage should be added to \Drupal\comment\Tests\CommentPagerTest

The page to view is calculated in \Drupal\comment\CommentStorage::getDisplayOrdinal

We call

$response->addCacheableDependency($subrequest_url);

In \Drupal\comment\Controller\CommentController::commentPermalink - I suspect we need to add

$response->getCacheableMetadata()->addCacheContexts('url');

after $response->addCacheableDependency($subrequest_url);

Working on it

looking for red/green

Hi @JonasMuc

FWIW, say hi to the community on IRC in #drupal-contribute, there is an active local user group in your part of the world - http://www.meetup.com/en-AU/Munchen-Drupal-User-Group-Meetup/

Lee

@JonasRMuc great find/report!

Overall this looks like hotfix, for long term fix suppose setting page argument twice on passed request is wrong

+++ b/core/modules/comment/src/Controller/CommentController.php
@@ -125,7 +125,7 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
-      $subrequest_url = $entity->urlInfo()->toString(TRUE);
+      $subrequest_url = $entity->urlInfo()->setOption('query', ['page' => $page])->toString(TRUE);
       $redirect_request = Request::create($subrequest_url->getGeneratedUrl(), 'GET', $request->query->all(), $request->cookies->all(), array(), $request->server->all());
       $redirect_request->query->set('page', $page);

Looks strange to pass page argument twice
Makes sense to check where pager searching for page...

Also #4 is a good pointer about cache contexts, if some contrib will provide comment settings (pager per user - like d6 was) then there should be a place to add other contexts

While we can cache this redirects (subrequests) we need to make sure that comments reordered when some comment deleted from this entity

I agree with @andypost's concerns. So… why don't we just let Request::create() handle the populating of $request->query for us? Then there's no duplication. Tests still pass.

+++ b/core/modules/comment/src/Tests/CommentPagerTest.php
@@ -12,6 +12,9 @@
+  public static $modules = ['dynamic_page_cache'];

This is not necessary; even the testing install profile installs this module by default.

Passed & failed as expected! :)

Thanx Win!

+++ b/core/modules/comment/src/Controller/CommentController.php
@@ -125,9 +125,8 @@ public function commentPermalink(Request $request, CommentInterface $comment) {
+      $subrequest_url = $entity->urlInfo()->setOption('query', ['page' => $page])->toString(TRUE);

Maybe we should we be doing setOption('query', $request->query->all()) instead of special casing page - thoughts?

Actually, nevermind, $page isn't in request->query - so this is fine - rtbc +1

Committed/pushed to 8.3.x and cherry-picked to 8.2.x. Thanks!