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
Comment | File | Size | Author |
---|---|---|---|
#23 | comment-pager.23.patch | 15.3 KB | larowlan |
#23 | interdiff.txt | 1.73 KB | larowlan |
#14 | interdiff.txt | 1022 bytes | larowlan |
#13 | comment-pager.doh_.patch | 15.32 KB | larowlan |
Comments
Comment #1
Wim LeersComment #2
andypostI 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
Comment #3
swentel CreditAttribution: swentel commentedCan'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.
Comment #4
Wim Leers#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?
Comment #5
andypost@swentel it would work if no other pagers are present on page ;(
Comment #6
dsnopekPager ids that are strings rather than integers?
Comment #7
Wim Leers#6: I think that's the only sane solution, yes.
Comment #8
larowlanHow does views do it?
Comment #9
larowlanWe could add a setting to the formatter 'pager id.' Just like views, put the burden on the user to ensure they use unique ids.
Comment #10
larowlanlike so
Comment #11
dsnopek@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.
Comment #13
larowlandoh, didn't include my last change in the patch.
Comment #14
larowlanwrong interdiff
Comment #15
Wim LeersWow. Impressive! larowlan's patch:
I hereby reward your hard work with a very grateful and very fast RTBC!
Comment #16
andypostThis 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.
This is a kind of bc API change, so fine
this limits us with 11 fields ;) ok
Comment #17
Berdir0 is empty, so that only adds it if pager_id > 0. By design? Maybe use isset() ? : 0; instead?
sorry for the nitpick, but I think optional must be lowercase.
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() ;)
Yes, here the !empty() check probably makes sense.
Comment #18
Wim Leers#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.
Comment #19
catchFeels 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?
Comment #20
BerdirYes, 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:
Comment #21
catchOK I can't think of anything else (except allowing string pager IDs, which we could apply to Views if so), so back to RTBC.
Comment #22
andypost@Lee do you really think that each formatter for comment field should provide this setting?
Comment #23
larowlanYes, by design - no need to add extra complexity/url length if not needed. Also hidden in the formatter settings until set.
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
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.
Comment #24
catchAsked 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!
Comment #26
andypostThe issue introduced bug in formatter see interdiff in #1875974-59: Abstract 'component type' specific code out of EntityDisplay