Updated: Comment #N

Problem/Motivation

#731724: Convert comment settings into a field to make them work with CMI and non-node entities made it possible to have multiple comment fields per entity type… yet it failed to update the pager to ensure each comment then has its unique pager.

That may seem like an exotic edge case, but without unique pagers, we cannot update the render cache key correctly either, hence it prevents correct code in #2151457: Comment pager breaks the render cache!

Sadly, to fix that seems to require pretty extensive changes to all of Drupal core's pager logic contained in pager.inc, which only allows one to assign a numeric identifier to a pager, to allow for multiple pagers on a page. But that's problematic too, because it makes it impossible (AFAICT) for us to figure out which pager belongs to which comments reliably.

So, the current sad reality is that the one ?page=X query will control all comment pagers simultaneously. And if one set of comments requires e.g. 2 pages and the other requires 5 pages… then you'll only be able to view 2 pages of either set of comments!

Proposed resolution

Fix comment pagers, possibly requiring improvements to the pager system in general.

Remaining tasks

TBD

User interface changes

None.

API changes

TBD

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Title: Multiple comment fields per entity are possible, but breaks horribly when they need pagers » Multiple comment fields per entity are supported, but break horribly when they need pagers
andypost’s picture

I got that bug in mind but still have no idea where to store global pager numbers.
Probably this require some page state/object to fetch/store pager id for each field but this approach is ugly and really require overhaul the paging subsystem a-la #33809: Make pagers not suck which lasts since 2005

swentel’s picture

Can't we just keep a static counter in viewElements() and augment that and pass it as the element to the pager build ? That should work fine - untested though.

Wim Leers’s picture

#3: AFAICT that doesn't allow us to figure out in e.g. #2151457: Comment pager breaks the render cache which pager is associated with which comment field?

andypost’s picture

Parent issue: » #33809: Make pagers not suck

@swentel it would work if no other pagers are present on page ;(

dsnopek’s picture

Pager ids that are strings rather than integers?

Wim Leers’s picture

#6: I think that's the only sane solution, yes.

larowlan’s picture

How does views do it?

larowlan’s picture

We could add a setting to the formatter 'pager id.' Just like views, put the burden on the user to ensure they use unique ids.

larowlan’s picture

Status: Active » Needs review
FileSize
15.32 KB

like so

dsnopek’s picture

@larowlan: That's a sweet workaround! One day it'd be great if those were machine names, rather than integers - but this will certainly work for now with out having to fix up the pager system.

Status: Needs review » Needs work

The last submitted patch, 10: comment-pager.1.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
0 bytes
15.32 KB

doh, didn't include my last change in the patch.

larowlan’s picture

FileSize
1022 bytes

wrong interdiff

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Performance, +D8 cacheability

Wow. Impressive! larowlan's patch:

  • Simple solution.
  • Extremely quickly implemented.
  • Manual testing verifies that this indeed solves the problem completely.
  • To top it off: full test coverage.
  • No noteworthy nitpicks either.
  • (I even tested with #2151457: Comment pager breaks the render cache + node/comment render caching enabled, and it worked perfectly: multiple comment pagers, and render caching worked correctly.)

I hereby reward your hard work with a very grateful and very fast RTBC!

andypost’s picture

This approach is a quick-fix, but actually if we have other "pager-able" stuff on page for example view, so comment thread still can get the pageNum from other object.

At the same time this approach sets the pager to limited set of ID that could be different for diferrent pages so does not totally solves the problem.

Also filed related #2156089: Remove comment_get_thread() in favour of method on CommentStorage this function is a heart of comment module.

  1. +++ b/core/modules/comment/comment.module
    @@ -694,9 +697,12 @@ function comment_add(EntityInterface $entity, $field_name = 'comment', $pid = NU
    +function comment_get_thread(EntityInterface $entity, $field_name, $mode, $comments_per_page, $pager_id = 0) {
    ...
    -function comment_get_thread(EntityInterface $entity, $field_name, $mode, $comments_per_page) {
    

    This is a kind of bc API change, so fine

  2. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
    @@ -191,4 +197,32 @@ public static function renderForm(array $context) {
    +    $element['pager_id'] = array(
    +      '#type' => 'select',
    +      '#title' => $this->t('Pager ID'),
    +      '#options' => drupal_map_assoc(range(0, 10)),
    

    this limits us with 11 fields ;) ok

Berdir’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
    @@ -127,11 +130,14 @@ public function viewElements(FieldItemListInterface $items) {
    +          }
    ...
    +          if (!empty($this->settings['pager_id'])) {
    +            $build['pager']['#element'] = $this->settings['pager_id'];
    

    0 is empty, so that only adds it if pager_id > 0. By design? Maybe use isset() ? : 0; instead?

  2. +++ b/core/modules/comment/comment.module
    @@ -636,6 +636,9 @@ function comment_add(EntityInterface $entity, $field_name = 'comment', $pid = NU
      *   The amount of comments to display per page.
    + * @param int $pager_id
    + *   (Optional) Pager id to use in case of multiple pagers on the one page.
    + *   Defaults to 0.
    

    sorry for the nitpick, but I think optional must be lowercase.

  3. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
    @@ -191,4 +197,32 @@ public static function renderForm(array $context) {
    +      '#options' => drupal_map_assoc(range(0, 10)),
    

    That kind of makes no sense :) range(0, 10) is not assoc, so calling drupal_map_assoc() on it has no effect.

    And if there's a reason for using it, then you should probably use \Drupal\Component\Utility\MapArray::copyValuesToKeys() ;)

  4. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
    @@ -191,4 +197,32 @@ public static function renderForm(array $context) {
    +  public function settingsSummary() {
    +    // Only show a summary if we're using a non-standard pager id.
    +    if (!empty($this->settings['pager_id'])) {
    

    Yes, here the !empty() check probably makes sense.

Wim Leers’s picture

#16: It's a quick fix, but I don't think it's reasonable to block this on fixing our Pager API. The API change is a necessity, was an oversight in the "comments as fields" issue, was rarely used anyway, and actually… all existing code continues to work just fine. So it's actually an API addition. 10 unique comment fields… that should be enough for everyone, no? :)
You're right that there could be yet other pagers on the same page. But *that* only fixable with a better Pager API.

#17: I'll leave these for larowlan to answer, I can only testify that I tested all this thoroughly manually and couldn't break things (I configured no pager (i.e. add comment field, but don't change settings), pager = 1, pager = 6, pager = 2, pager = 0 — so I pretty much tested everything).
I'd prefer a minor follow-up with those improvements if a committer gets to this before larowlan can do a reroll.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Feels like we ought to be able to automatically increment the pager ID. I'd expect views pagers to do this so maybe check there for the implementation?

Berdir’s picture

Yes, I guess it could only break if yet another pager is used on the same page and comment uses pager_id 0. It just seems inconsistent to add the pager id/element always to the query but to the theme function only if it's not 0.

@catch: Views seems to handle it in exactly the same way:

    $form['id'] = array(
      '#type' => 'number',
      '#title' => t('Pager ID'),
      '#description' => t("Unless you're experiencing problems with pagers related to this view, you should leave this at 0. If using multiple pagers on one page you may need to set this number to a higher value so as not to conflict within the ?page= array. Large values will add a lot of commas to your URLs, so avoid if possible."),
      '#default_value' => $this->options['id'],
    );
catch’s picture

Status: Needs work » Reviewed & tested by the community

OK I can't think of anything else (except allowing string pager IDs, which we could apply to Views if so), so back to RTBC.

andypost’s picture

Assigned: Unassigned » larowlan

@Lee do you really think that each formatter for comment field should provide this setting?

larowlan’s picture

FileSize
1.73 KB
15.3 KB

0 is empty, so that only adds it if pager_id > 0. By design?

Yes, by design - no need to add extra complexity/url length if not needed. Also hidden in the formatter settings until set.

That kind of makes no sense :) range(0, 10) is not assoc, so calling drupal_map_assoc() on it has no effect.

Ha, old habits die hard - yeah 0-10 is already keyed 0-10, if it were 1-10, it would have made sense. So yeah the wrapping with drupal_map_assoc isn't needed - fixed that and the Optional

do you really think that each formatter for comment field should provide this setting

I'm not sure what the alternative is, I guess it could be a field setting, but our goal is to get the per page settings into the formatter too, so this would fit well with that.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Asked on irc about this, thinking that Views had its own way to fix the multiple pagers issue.

Turns out views also has a setting for pager ID. So until we have string pager IDs, this is the only way around the issue. There's already an issue to fix pages, so committing this one as is. Thanks!

Status: Fixed » Closed (fixed)

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

andypost’s picture

The issue introduced bug in formatter see interdiff in #1875974-59: Abstract 'component type' specific code out of EntityDisplay