Having multiple node access grants within the same realm breaks comment pager query

Fixed the notices. Hope it fails for the reason it should, now.

Experimenting ...

Edit: This makes sure that it's actually viewing the node page and has access to the comments section. Getting crazy here ... but it passed again. Now I can only hope it fails on your machine, too.

One last try for tonight: Create 60 comments so that we can assert that two pages exists, but no third.

In the mean-time: These look similar: #135722: Taxonomy Access Control causes paging error on front page (Drupal 5) closed as duplicate of #668268: Weird behavior of pager for non-admin users (Drupal 6), where someone explains, that it has to do with the role count.

Hehe, think I've got it now. I am using clean URLs locally, so testing for ?page= is fine, but the testbot has no clean URLs and thus &page=. Using just page= now.

Yay! Just as it should.

This is the query after all altering:

SELECT DISTINCT COUNT(*) AS expression
FROM 
{comment} c
INNER JOIN {node_access} na ON na.nid = c.nid
WHERE 
  (c.nid = 1) AND (c.status = 1)
  AND (
    ( (na.gid = 0) AND (na.realm = all) )
    OR( (na.gid = 2) AND (na.realm = taxonomy_access_role) )
    OR( (na.gid = 8) AND (na.realm = taxonomy_access_role) )
    OR( (na.gid = 9) AND (na.realm = taxonomy_access_role) )
  )AND (na.grant_view >= 1)

Port of the test to Drupal 8 core with a little debugging in the pager attached.

And the debug output:

'SELECT DISTINCT COUNT(*) AS expression
FROM 
{comment} c
INNER JOIN {node_access} na ON na.nid = c.nid
WHERE  (c.nid = :db_condition_placeholder_0) AND (c.status = :db_condition_placeholder_1) AND(( (na.gid = :db_condition_placeholder_2) AND (na.realm = :db_condition_placeholder_3) )OR( (na.gid = :db_condition_placeholder_4) AND (na.realm = :db_condition_placeholder_5) )OR( (na.gid = :db_condition_placeholder_6) AND (na.realm = :db_condition_placeholder_7) ))AND (na.grant_view >= :db_condition_placeholder_8) '


array (
  ':db_condition_placeholder_0' => '1',
  ':db_condition_placeholder_1' => 1,
  ':db_condition_placeholder_2' => 0,
  ':db_condition_placeholder_3' => 'all',
  ':db_condition_placeholder_4' => '2',
  ':db_condition_placeholder_5' => 'node_access_test_author',
  ':db_condition_placeholder_6' => 8888,
  ':db_condition_placeholder_7' => 'node_access_test',
  ':db_condition_placeholder_8' => 1,
)

Thought about that one. No problem since we're building our own count query anyway.

I like it. This is a hack for sure, but not a bad one to solve the issue in D7. In D8 #778050: Add support for database hints and make PagerDefault properly pluggeable might change paged queries, so that we no longer need a special count query for MySQL and PostGres and might fallback on subqueries for SQLite.

About the position of the groupBy: Even if we have the node_access_groupby tag, can we be sure the node_access table is always joined?

Having admin/content/comment, I am looking how that is build now. On a default setup, where you have the privileges to see that, you often have bypass access privileges, too, so problems there would be hard to notice.
No problem there, that view is using the automatically generated subquery solution, instead of a custom count query.

Wrong issue again ... uploading the files anyway to avoid having to redo it in the other issue.