When a comment is submitted, any pager links rendered in the comment field will use the ajax path instead of the path of the node/entity that the comment field is attached to.

Steps to reproduce:
1. Create a node with comment field, ajax comments enabled, set comments per page in the field settings to 10, add 10 comments.
2. Add an 11th comment, observe that the comment is successfully submitted, the comment field is re-rendered via AJAX and has a pager.
3. Observe that the pager links are incorrect.

There are 2 parts to this issue. The first is that when the render array for the comment field is created in renderCommentField, the pager uses the default #route_name, which defaults to , using the relative path from which the pager is rendered. Because this is called within AJAX, it's using the AJAX path instead of the path of the parent node.

The second part of this issue has to do with unnecessary parameters that are being tacked on to the pager links when the pager is rendered within buildCommentFieldResponse (the render array is rendered to an HTML string when it is passed to create a new ReplaceCommand, AjaxCommentsController.php, line 157).

The parameter `_wrapper_format=drupal_ajax' attached to pager links breaks them.

CommentFileSizeAuthor
#32 interdiff_31-32.txt1.84 KBbeachston
#32 ajax_comments-pager_fix_with_comment_parent_entity-2998613-32.patch1.07 KBbeachston
#31 ajax_comments-pager_fix_with_comment_parent_entity-2998613-31.patch1.75 KBbeachston
#27 interdiff_22-27.txt1.43 KBbeachston
#27 ajax_comments-fix_pager_urls-2998613-27.patch7.92 KBbeachston
#23 interdiff_21-22.txt700 bytesbeachston
#23 ajax_comments-fix_pager_urls-2998613-22.patch7.76 KBbeachston
#21 ajax_comments-fix-pager-urls-2998613-21.patch7.83 KBbeachston
#15 ajax_comments-fix_pager_urls-2998613-15.patch7.43 KBbeachston
#13 ajax_comments-fix_pager_urls-2998613-13.patch7.35 KBbeachston
ajax-comments-pager-fix.patch2.13 KBbeachston
#3 ajax_comments-fix_pager_urls-2998613-d8.patch3.84 KBbeachston
#4 ajax_comments-fix_pager_urls-2998613-3.patch32.67 KBbeachston
#4 ajax_comments-pager_test-2998613-3.patch2.74 KBbeachston

Comments

beachston created an issue. See original summary.

danmuzyka’s picture

danmuzyka commented
Status: Active » Needs work

Hey Ryan, thanks for posting this!

  1. +++ b/src/Controller/AjaxCommentsController.php
@@ -152,7 +163,14 @@ class AjaxCommentsController extends ControllerBase {
+    // When the render array $comment_display is rendered, the ajax query parameter is added
+    // to the pager links. There isn't a good way to remove that (TODO write a custom formatter for comments maybe?)
+    // so instead, this pre-renders the comment field and removes the parameter from the html string.

    Thanks for adding these detailed comments! Could you reformat them so that no line exceeds 80 characters, per Drupal coding standards? 

  2. +++ b/src/Controller/AjaxCommentsController.php
@@ -152,7 +163,14 @@ class AjaxCommentsController extends ControllerBase {
+    $rendered_comment_display = \Drupal::service('renderer')->renderRoot($comment_display);
+    $rendered_comment_display = str_replace('_wrapper_format=drupal_ajax&page', 'page', $rendered_comment_display);

    I still think it would be worth exploring a method of doing this that doesn't rely on string replace.

    Also, in general, could you inject the services into the constructor and the create() method rather than using the procedural \Drupal::service() static method here? 

  3. +++ b/src/Controller/AjaxCommentsController.php
@@ -122,6 +122,17 @@ class AjaxCommentsController extends ControllerBase {
+    $router = \Drupal::service('router.no_access_checks');

    Could you inject the services into the constructor and the create() method rather than using the procedural \Drupal::service() static method here?

     

  4. Could you add an automated test for this issue? Let me know if you need help with that.
beachston’s picture

beachston commented
StatusFileSize
new ajax_comments-fix_pager_urls-2998613-d8.patch3.84 KB
beachston’s picture

beachston commented
StatusFileSize
new ajax_comments-pager_test-2998613-3.patch2.74 KB
new ajax_comments-fix_pager_urls-2998613-3.patch32.67 KB

Included are patches with

1. (test) Just the updated test case
2. Updated test case and fix

beachston’s picture

beachston commented
Status: Needs work » Needs review

The last submitted patch, 4: ajax_comments-fix_pager_urls-2998613-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 4: ajax_comments-pager_test-2998613-3.patch, failed testing. View results

The last submitted patch, 4: ajax_comments-pager_test-2998613-3.patch, failed testing. View results

The last submitted patch, 4: ajax_comments-fix_pager_urls-2998613-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

danmuzyka’s picture

danmuzyka commented
Status: Needs review » Needs work 
  1. +++ b/src/Controller/AjaxCommentsController.php
@@ -64,13 +74,16 @@ class AjaxCommentsController extends ControllerBase {
+   * @param \Drupal\Core\Routing\AccessAwareRouter $router

    Here you are using the type \Drupal\Core\Routing\Router, but in your constructor you are using \Drupal\Core\Routing\AccessAwareRouter, which actually doesn't extend the other class. The common interface that each of them implement is:

    \Symfony\Component\Routing\RouterInterface

    However, the method you invoke later in the code is match(), which is declared in Drupal\Core\Routing\AccessAwareRouterInterface, so I would recommend using that in both your class member declaration and your constructor. 

  2. +++ b/tests/src/FunctionalJavascript/AjaxCommentsFunctionalTest.php
@@ -20,85 +20,104 @@ use Drupal\Core\Entity\Entity\EntityViewDisplay;
+            'type' => 'article',

    Some indentation errors that still need cleanup to match Drupal coding standards.

I'll look at this again once you get the test to pass. Thanks!!

rudranil29’s picture

rudranil29 commented
Assigned: Unassigned » rudranil29
rudranil29’s picture

rudranil29 commented
Assigned: rudranil29 » Unassigned
beachston’s picture

beachston commented
Status: Needs work » Needs review
StatusFileSize
new ajax_comments-fix_pager_urls-2998613-13.patch7.35 KB

Attached is an updated patch with the updated test.

Looking at core/includes/pager.inc, it doesn't appear to me that there is any good way to get to that '_wrapper_format=ajax' parameter that breaks the pager before it is rendered to a string, since this happens in template_preprocess_pager and any hooks on the pager would be run after that is called. So this fix still uses str_replace to remove it, but in a preprocess function rather than after the entire comment field render array is rendered to Markup in buildCommentFieldResponse.

I also fixed the hardcoded '/node/' in the renderCommentField function to use the entity to get the Route/Route parameters, so this fix should work for any entity using ajax comments, not just nodes.

Status: Needs review » Needs work

The last submitted patch, 13: ajax_comments-fix_pager_urls-2998613-13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

beachston’s picture

beachston commented
StatusFileSize
new ajax_comments-fix_pager_urls-2998613-15.patch7.43 KB

Fixed some coding standards violations, as well as checking that the entity actually has a path. The test runs fine locally but I think it is getting hung up when a comment is a reply to another comment.

beachston’s picture

beachston commented

Fixed some coding standards violations, as well as empty checks for entity route and pager links

beachston’s picture

beachston commented

Fixed some coding standards violations, as well as empty checks for entity route and pager links

beachston’s picture

beachston commented
beachston’s picture

beachston commented
Status: Needs work » Needs review

Fixed some coding standards violations, as well as empty checks for entity route and pager links

Status: Needs review » Needs work

The last submitted patch, ajax_comments-fix_pager_urls-2998613-15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

beachston’s picture

beachston commented
StatusFileSize
new ajax_comments-fix-pager-urls-2998613-21.patch7.83 KB

Uses RouteProviderInterface to retrieve the route instead of getting it directly from the entity URL

beachston’s picture

beachston commented
beachston’s picture

beachston commented
StatusFileSize
new ajax_comments-fix_pager_urls-2998613-22.patch7.76 KB
new interdiff_21-22.txt700 bytes
beachston’s picture

beachston commented
Status: Needs work » Needs review
beachston’s picture

beachston commented

Adding a comment since a patch with this comment number was already submitted

beachston’s picture

beachston commented

Adding a comment since a patch with this comment number was already submitted

beachston’s picture

beachston commented
StatusFileSize
new ajax_comments-fix_pager_urls-2998613-27.patch7.92 KB
new interdiff_22-27.txt1.43 KB

This updated patch (27) should resolve coding standards errors introduced by the previous patch (22)

danmuzyka’s picture

danmuzyka commented
Status: Needs review » Reviewed & tested by the community

This looks good to me!

  • beachston authored fd41c39 on 8.x-1.x 
    Issue #2998613 by beachston, danmuzyka: Pager URLs are broken
danmuzyka’s picture

danmuzyka commented
Status: Reviewed & tested by the community » Fixed

I've merged this into the 8.x-1.x branch.

beachston’s picture

beachston commented
StatusFileSize
new ajax_comments-pager_fix_with_comment_parent_entity-2998613-31.patch1.75 KB

After running into an instance on a client project where the approved patch still resulted in errors, I've given this issue another look. It seems to me that the using 

$entity_url = $entity->toURL();
$path = $entity_url->toString();

should work for any entity type except when the ajax comment is a reply to another comment, in which case $entity, as it is passed in the function renderCommentField is the parent comment and not the actual parent entity. This patch adds an exception for comments. I don't know the full implications if comments are added to a custom entity but my assumption is that custom entities are given some default path, i.e. calling $entity->toURL() on a custom entity returns the default entity/entityType/id.

beachston’s picture

beachston commented
Status: Fixed » Needs review
StatusFileSize
new ajax_comments-pager_fix_with_comment_parent_entity-2998613-32.patch1.07 KB
new interdiff_31-32.txt1.84 KB

Using entity->toURL is still causing a test failure on D.O, patch 32 is a copy of the approved patch (27) with the comment reply exception code included

  • beachston authored 42cb533 on 8.x-1.x 
    Issue #2998613 by beachston, danmuzyka: Pager URLs are broken
danmuzyka’s picture

danmuzyka commented
Status: Needs review » Fixed

Thanks @beachston, I've merged your updated patch into the 8.x-1.x branch.

Status: Fixed » Closed (fixed)

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