Error: Call to a member function getTotalCount() on boolean in statistics_get()

Reviewing the list of commits to statistics module since August 1, seems like the most likely culprit is probably #1446932: Improve statistics performance by adding a swappable backend, as it touched the statistics_get() function.

+function statistics_get($id) {
+  if ($id > 0) {
+    /** @var \Drupal\statistics\StatisticsViewsResult $statistics */
+    $statistics = \Drupal::service('statistics.storage.node')->fetchView($id);
+    return [
+      'totalcount' => $statistics->getTotalCount(),
+      'daycount' => $statistics->getDayCount(),
+      'timestamp' => $statistics->getTimestamp(),
+    ];
   }
 }
....

+  public function fetchView($id) {
+    $views = $this->fetchViews(array($id));
+    return reset($views);
+  }

That reset() call will return FALSE if $views is an empty array, which it would be if it were checking against a node ID that no longer existed, for example.

I see this call to statistics_get() is coming from your theme; is it possible it's checking against an expired node ID? Or is this happening on every page?

Thanks for reporting this.

I'll try to get a fix tomorrow if no one else has by then. I think we just need to put a few checks in place.

We need some test coverage.

I have a test; will post shortly.

Just a note I thought we'd seen something like this before, but it wasn't, it was #2616330: statistics.php can exit early and prevent output errors which won't help here - in case anyone has the same thought.

So yeah, this is a small BC break for statistics_get(). In 8.2.x it would have returned FALSE for an invalid node ID; in 8.3.x/8.4.x it fatals. Attached demonstrates the fatal. I also manually removed a couple lines of context to prove it works on 8.2.x.

And with the fix. This is why we want deprecated test coverage (both dedicated tests for deprecated code and tests to check for deprecated code).

Adding a more explicit comment in the test. This probably still won't stop someone from converting it indiscriminately, but it at least should stop us from committing their indiscriminate conversion.

Instead of checking its empty should be instead check it's a StatisticsViewsResult object.

Sure, that makes sense. More robust that way BC-wise.

+++ b/core/modules/statistics/src/Tests/StatisticsLoggingTest.php
@@ -125,6 +126,17 @@ public function testLogging() {
+
+    // Try fetching statistics for an invalid node ID and verify it returns
+    // an empty array.

Should be FALSE rather than empty array no?

Otherwise looks good.

Right.

Awesome turnaround

This will get NW in a bit once that interdiff gets tested. ;)

Probably should wait for CI in any case.

As above.

@connectedrobots Are you able to manually test the patch in comment #19?

The test-only patch in #8 also shows the exact fatal BTW (in the console output):

20:31:59 Fatal error: Call to a member function getTotalCount() on boolean in /var/www/html/core/modules/statistics/statistics.module on line 129

Didn't mean to leave this assigned to myself.

Very minor docs improvements so other consumers of the new API might expect it to work this way. Fortunately, statistics_node_links_alter() does it right.

+++ b/core/modules/statistics/src/StatisticsStorageInterface.php
@@ -27,7 +27,10 @@ public function recordView($id);
-   * @return array \Drupal\statistics\StatisticsViewsResult
+   * @return array \Drupal\statistics\StatisticsViewsResult[]
+   *   An array of value objects representing the number of times each entity
+   *   has been viewed. The array is keyed by entity ID. If an ID does not
+   *   exist, it will not be present in the array.

The words and the datatype here do not agree -- datatype says it's a nested array.

Edit: Or I guess the datatype is not valid phpdoc? So probably a typo in fixing the typo.

BTW not entirely sure the bad docs are in scope to begin with; while they could cause this bug in other code, they probably didn't in statistics_get() since that was in the same patch where they were added.

(And also statistics_get() not retaining BC is its own bug regardless of what API replaces it.)

I dunno it was not being aware of this whilst converting statistics_get() to use the new API that caused the bug. If we'd had the correct docs you can programmatically work out where code doesn't code for all possibilities. PHPStorm for example can be set up to do this.

Okay.

I guess I can RTBC this since the docs changes correlate to what I saw in the patch @timmillwood previously RTBCed and it's a docs addition/fix. Someone other than me or @alexpott should commit the patch though.

Thank you everyone!

Committed c2f6edd and pushed to 8.4.x. Cherry-picked as 3689c11 and pushed to 8.3.x.

Generally-speaking, always go for the latest patch, which in this case is #27.

However! In this case, this change was already made to the code base, so you should be able to upgrade to the 8.3.x-dev release and see/test the fix.

The patch is against the root of the Drupal core directory rather than the statistics module, try applying from there.

8.3.x dev release is here:
https://www.drupal.org/project/drupal/releases/8.3.x-dev

All core patches are created with respect to the base of the Drupal installation, not /core. So, I think, in this case you should be in directory "www", not "www/core" when you apply the patch.

Hi @connectedrobots,

In the short term, updating to the current 8.3.x-dev release as @catch above linked it would probably be a good way to ensure that the bug does not resurface, especially if you are unsure whether or not the patch was applied or about maintaining a site with a patch running. If you do use the development release, just be sure to update it again to the latest dev code whenever there are announcements of new releases, especially security releases.

The next scheduled bugfix release for Drupal core is May 3. There could be other releases between now and then that might not include this fix, but the May 3 release almost certainly will. So after May 3 you should be able to update Drupal to a stable release and count on Drupal updates to work normally again after that. Just keep an eye on the release notes for any release between now and then. The first release that includes this fix will mention it prominently in the release notes.

I hope this helps! Thanks again for reporting this issue right away.