$total_items can be negative if pager has offset

It would be nice if we have some form of test coverage here?

make a new testcase for this in PagerPluginBaseTest?
like:
testExecuteCountQueryWithOffsetLargerThanResult()

This sounds super nice!

Looks good to me, let's see what the bot says...

Test on #6 fails with:

Failed asserting that -1 matches expected 0.

So this tells me that the test is indeed correct.

Here is both the fix and the test combined into one, against 8.4.x

1. +++ b/core/modules/views/src/Plugin/views/pager/PagerPluginBase.php
   @@ -157,6 +157,11 @@ public function executeCountQuery(&$count_query) {
   +    // Prevent from being negative.
   +    if ($this->total_items < 0) {
   +      $this->total_items = 0;
   +    }
   

   We could do this in a one-liner

   $this->total_items = max(0, $this->total_items);
   

   - not sure it matters though

2. +++ b/core/modules/views/tests/src/Unit/Plugin/pager/PagerPluginBaseTest.php
   @@ -247,6 +247,30 @@ public function testExecuteCountQueryWithOffset() {
   +    $statement = $this->getMock('\Drupal\Tests\views\Unit\Plugin\pager\TestStatementInterface');
   ...
   +    $query = $this->getMockBuilder('\Drupal\Core\Database\Query\Select')
   

   Can we use the ::class constants here. It makes discovery of dependencies more explicit, e.g. IDEs will find, but they're less likely to find these strings.

Needs work for item 2 of #12. Not fussed either way on item 1.

I tend to agree with @keesje here... a quick grep across core also shows that this is a very common pattern.
Perhaps tackling this problem on a different issue for all core tests would make sense?

Makes sense @vaplas

if the committers consider this change to be out of scope, we can easily move it to a new issue (or offer it to Novice for improve skills)

It came to mind that @larowlan is now a committer so that settles it then :-D

So, fixed 1 of #12 and not 2.

@pk188 can you please provide an interdiff for #19?

Here it is @larowlan
@pk188 you can have a look at how to create these here https://www.drupal.org/documentation/git/interdiff :)

Oops, wrong file extension (should be .txt) sorry test bot!

Interdiff @larowlan. Sorry for being late.

@pk188 - no need to apologize, thanks for working on this bug.

Can we get the change to use ::class constants done here too, easier to do it before than later.

+++ b/core/modules/views/src/Plugin/views/pager/PagerPluginBase.php
@@ -157,10 +157,7 @@
-    // Prevent from being negative.

Can we get this comment added back?

This is very close, thanks everyone for accommodating my feedback.

Fixed #27.

@pk188 thanks again - please look after your reviewers by providing an interdiff, so they don't have to review everything from scratch :)

+++ b/core/modules/views/tests/src/Unit/Plugin/pager/PagerPluginBaseTest.php
@@ -247,6 +247,30 @@ public function testExecuteCountQueryWithOffset() {
+    $statement = $this->getMock('\Drupal\Tests\views\Unit\Plugin\pager\TestStatementInterface');
...
+    $query = $this->getMockBuilder('\Drupal\Core\Database\Query\Select')

These still need to use ::class constants

#15 done #2886352: Replace string 'path/to/class' with ::class constant :)
This can be used here like hint.

Yay!

Switching the test to use ::class constants.

Looks good to me

Committed/pushed to 8.4.x, thanks!