Improve statistics performance by adding a swappable backend

jStats does something similar to your proposal (doesn't use a cache table though)
http://drupal.org/project/jstats

The trick is to replace "N updates" with "N inserts + 1 update" which is known much cheaper in MySQL. It may be not correct/applicable for another backend.

@timmillwood I don't see why a

SELECT * FROM node_counter WHERE nid = :nid

is not as good as

SELECT * FROM cache_node_counter WHERE cid = :cid

The first one is even better in some db system, as nid is an integer. The second is only better with another cache backend other than the core default one.

I'm starting to play around with a variant of #4 with Drupal 6. It's working well, and my take is to set the cache expiration via a variable, then only cache for the current time + that variable. I set it to 15 minutes or 900 seconds by default. So I'm not clearing at all via cron, just allowing the cache to expire naturally.

I'd move to another stats module, but I have a bunch of stuff tied to core statistics so it's easier for me to just improve it than rethink everything else.

Here is what my D6 version looks like for reference (not that it will ever have a chance of being added.) One note to anyone else that tries to use it, my version of Drupal 6 has a backport of REQUEST_TIME which is not in D6 by default. If you try to use this you need to replace that with time().

I'll probably play with drupal_queue backport next with the other patches.

In the tests, you should use

$this->cronRun();

instead of calling the URL on your own.
(Btw: cron.php was nuked recently, so that will be the cause of some of the failures.)

Some (mostly) minor clean-up stuff:

+++ b/core/modules/search/search.test
@@ -459,6 +459,8 @@ class SearchRankingTestCase extends SearchWebTestCase {
+    $key = variable_get('cron_key', 'drupal');
+    $this->drupalGet($base_url . '/core/cron.php', array('external' => TRUE, 'query' => array('cron_key' => $key)));

Leftover call to old cron.php

+++ b/core/modules/statistics/statistics.module
@@ -231,6 +231,21 @@ function statistics_user_predelete($account) {
+  ¶

+++ b/core/modules/statistics/statistics.test
@@ -373,38 +378,39 @@ class StatisticsAdminTestCase extends DrupalWebTestCase {
+    ¶
...
+    ¶
...
+    ¶
...
+    ¶

Trailing whitespace.

+++ b/core/modules/statistics/statistics.php
@@ -14,20 +14,12 @@ chdir('../../..');
-drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES);
+drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

This seems like it could actually be a perfomance hit, even if it's only a small one. Should this be benchmarked for people with the default queueing backend?

+++ b/core/modules/statistics/statistics.test
@@ -96,6 +96,7 @@ class StatisticsLoggingTestCase extends DrupalWebTestCase {
+    global $base_url;

@@ -232,9 +234,9 @@ class StatisticsReportsTestCase extends StatisticsTestCase {
+    global $base_url;

@@ -361,6 +365,7 @@ class StatisticsAdminTestCase extends DrupalWebTestCase {
+    global $base_url;

@@ -373,38 +378,39 @@ class StatisticsAdminTestCase extends DrupalWebTestCase {
+    global $base_url;

@@ -465,21 +473,20 @@ class StatisticsAdminTestCase extends DrupalWebTestCase {
+    global $base_url;

@@ -519,6 +526,7 @@ class StatisticsTokenReplaceTestCase extends StatisticsTestCase {
+    global $base_url;

This is not needed now that we use $this->drupalCron()

I dont see the point of this. We're just trading one INSERT for another. This assumes that you have a high performance (non-mysql) queue backend and that's why it's a better option. I think a better approach would be to have the stats backend be swappable.

What if we were to move it all into the Statistics module itself? Also don't think we need StatisticsBackend wrapper since you can just re-register "statistics" with a different class if you wanted.

Had the wrong namespace in the interface.

Huh?

+++ b/core/modules/statistics/src/StatisticsBackend.php
@@ -0,0 +1,38 @@
+class StatisticsBackend {

Some interface could we certainly useful, otherwise, well it would be totally pointless :)

Rerolling...

1. +++ b/core/modules/statistics/src/StatisticsStorage.php
   @@ -0,0 +1,106 @@
   +class StatisticsStorage implements StatisticsStorageInterface {
   

   What about naming the class to make it clear that its for database ... what about using StatisticsDatabaseStorage

2. +++ b/core/modules/statistics/src/StatisticsStorage.php
   @@ -0,0 +1,106 @@
   +  if (in_array($dbfield, array('totalcount', 'daycount', 'timestamp'))) {
   ...
   +  }
   +  return FALSE;
   

   Do you mind indenting this :)

3. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,86 @@
   +/**
   + * Provides an interface defining Statistics Storage
   + */
   

   A little bit of docs about what is stored in the stats would be great

4. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,86 @@
   +  public function recordHit($nid);
   

   I still don't get why we don't make some more useful interface, given that we have a completely new API anyway.

Removing the "statistics_title_list" function and calling the storage service directly from the block plugin.

This sounds like a unnecessary API removal.

What were you thinking? Maybe better as a new issue? Already thinking of making. A new patch to make it entity counter rather than node counter.

Well, I think we should avoid API breakage for things we introduce ....

At this stage in the cycle we probably have to keep the old functions and mark @deprecated. But be aggressive in what we mark @deprecated now that there is a service to use instead.

1. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -58,18 +58,20 @@ public function defaultConfiguration() {
   +
   +    $storage = \Drupal::service('statistics.statistics_storage');
   

   This isn't a static method, so inject the service properly. \Drupal:: inside a class is a code smell, always.

2. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,106 @@
   +    return (bool) $this->connection->merge('node_counter')
   +      ->key('nid', $nid)
   +    ->fields(array(
   +      'daycount' => 1,
   +      'totalcount' => 1,
   +      'timestamp' => REQUEST_TIME,
   +    ))
   +      ->expression('daycount', 'daycount + 1')
   +        ->expression('totalcount', 'totalcount + 1')
   +          ->execute();
   

   The whitespacing here is AFU. :-) Please standardize. (Same in other methods, too. Looks like default indentation from an IDE that wasn't cleaned up.)

3. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,106 @@
   +    if ($nid > 0) {
   

   Is this really a check that's needed? If so, sounds more like an assertion type check.

4. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,106 @@
   +      // Retrieve an array with both totalcount and daycount.
   

   Except there's a 3rd value here that's also returned.

5. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,106 @@
   +    $q = $this->connection->select('node_counter', 'nc');
   +    $q->addExpression('MAX(totalcount)');
   +    return (int) $q->execute()->fetchField();
   

   Conventionally we're using $query, not $q, for query objects. Single-character variable names are almost never useful.

6. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,88 @@
   +   * Returns if node view is counted
   

   Period at the end of the line. (Same for other docblocks.)

   Also, this description is wrong. recordHit() updates data. The text here suggests its just a reporting tool, which is not at all the case. Don't just duplicate the @return doc unless that's really *all* that happens.

7. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,88 @@
   +   *   The id of the node to count
   

   Ibid.

8. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,88 @@
   +   * Returns if day count is reset
   

   As above, this method *modifies data*. The summary line should say why you'd call it, not replicate @return.

9. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,88 @@
   +   * @return SelectQuery|FALSE
   +   *   A query result containing the node ID, title, user ID that owns the node,
   +   *   and the username for the selected node(s), or FALSE if the query could not
   +   *   be executed correctly.
   

   Don't return a DB object. That's breaking encapsulation. Return just the data you're asking for.

   Looking at it again... is this even a relevant method for the interface? Seems like an internal implementation detail of the DB implementation (and thus should be only a protected method).

10. +++ b/core/modules/statistics/statistics.module
    @@ -68,61 +68,21 @@ function statistics_node_links_alter(array &$node_links, NodeInterface $entity,
    +  $storage = \Drupal::service('statistics.statistics_storage');
       $statistics_timestamp = \Drupal::state()->get('statistics.day_timestamp') ?: 0;
    

    If the state system is always updated at the same time as the service, should it not be internalized into the service?

    if ($storage->needsReset()) {
    $storage->resetDayCount();
    }

    That needsReset is pulling from the state API under the hood is not this hook's business.

11. +++ b/core/modules/statistics/statistics.module
    @@ -68,61 +68,21 @@ function statistics_node_links_alter(array &$node_links, NodeInterface $entity,
    +    // Reset day counts
    +    $storage->resetDayCount();
    

    Comment is now redundant.

12. +++ b/core/modules/statistics/statistics.module
    @@ -68,61 +68,21 @@ function statistics_node_links_alter(array &$node_links, NodeInterface $entity,
       // Calculate the maximum of node views, for node search ranking.
    -  \Drupal::state()->set('statistics.node_counter_scale', 1.0 / max(1.0, db_query('SELECT MAX(totalcount) FROM {node_counter}')->fetchField()));
    +  $max_total_count = $storage->maxTotalCount();
    +  \Drupal::state()->set('statistics.node_counter_scale', 1.0 / max(1.0, $max_total_count));
    

    Same here. Let's hide the state API usage behind the service as well. Then ask how much should be shared between possible implementations of that service.

13. +++ b/core/modules/statistics/statistics.module
    @@ -137,10 +97,9 @@ function statistics_title_list($dbfield, $dbrows) {
     function statistics_get($nid) {
    -
       if ($nid > 0) {
    -    // Retrieve an array with both totalcount and daycount.
    -    return db_query('SELECT totalcount, daycount, timestamp FROM {node_counter} WHERE nid = :nid', array(':nid' => $nid), array('target' => 'replica'))->fetchAssoc();
    +    $storage = \Drupal::service('statistics.statistics_storage');
    +    return $storage->fetchViews($nid);
       }
     }
    

    Seems this function should now be marked @deprecated? It's a one liner now, or should be. (return Drupal::service()->fetchViews());

1. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -14,13 +15,23 @@ class StatisticsDatabaseStorage implements StatisticsStorageInterface {
      /**
   +   * The state service.
   +   *
   +   * @var \Drupal\Core\State\StateInterface
   +   */
   +  protected $state;
   +  ¶
   +  /**
   

   Dreditor says there's a stray whitespace on line 24.

2. +++ b/core/modules/statistics/statistics.module
   @@ -69,37 +69,30 @@ function statistics_node_links_alter(array &$node_links, NodeInterface $entity,
   + * @deprecated in Drupal 8.0.x
   

   @deprecated markers should indicate what to use instead.

3. +++ b/core/modules/statistics/statistics.module
   @@ -69,37 +69,30 @@ function statistics_node_links_alter(array &$node_links, NodeInterface $entity,
   +function statistics_title_list($dbfield, $dbrows) {
   +  return \Drupal::service('statistics.statistics_storage')->statisticsTitleList($dbfield, $dbrows);
   +}
   

   This is now calling a method that is not on the interface, which means implicitly coupling it to a single implementation. You don't want that. :-)

   Stepping back, what's the use of this function/method? How can it be described in a non-DB-dependent way?

#82: Here's the problem. Say you write a MongoDB implementation of the storage class. Now statisticsTitleList returns... what? Having it return an SQL-related object makes no sense at all, and is probably not even feasible. That's not going to fly.

Tim: What is the data that a caller needs to get back from that method, and how would they use it? Specifically, can you describe it in a way that does not involve the word "result" or "database"?

1. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +177,37 @@ public function build() {
   +    $nodes = $this->entityManager->getStorage('node')->loadMultiple($counts);
   +
   +    $items = array();
   +    foreach ($counts as $count) {
   +      $items [] = \Drupal::l($nodes[$count]->getTitle(), new Url('entity.node.canonical', ['node' => $count], array()));
   

   Is there a reason to not directly use $node->urlInfo()

2. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +177,37 @@ public function build() {
   +      '#cache' => ['tags' => Cache::mergeTags(['node_list'],Cache::buildTags('node', $counts))]
   

   Nitpick: Missing space after 'node_list'],

3. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,111 @@
   +
   +class StatisticsDatabaseStorage implements StatisticsStorageInterface {
   

   Needs some docs, if possible

4. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,111 @@
   +    return (REQUEST_TIME - $statistics_timestamp) >= 86400;
   ...
   +    $this->state->set('statistics.day_timestamp', REQUEST_TIME);
   

   If you want you can also use $request->server->get('REQUEST_TIME')

Getting super close! :-)

1. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +176,37 @@ public function build() {
   +  protected function nodeTitleList($counts, $title){
   

   Space needed before {

2. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +176,37 @@ public function build() {
   +      $items [] = \Drupal::l($nodes[$count]->getTitle(), $nodes[$count]->urlInfo('canonical'));
   

   Drupal::l() should be considered deprecated (even though not officially). The preferred way to pass around links is to use the Url object, which can safely be put into a render array and handles its own cache tag information.

   And even baring that, \Drupal:: calls inside a class are a no-no. :-) Inject the generator service yourself.

3. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,118 @@
   +    // Retrieve an array with both totalcount, daycount and timestamp.
   

   Nitpick: Both implies 2 values, not 3. Also, Drupal uses an Oxford comma like all civilized people.

4. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,90 @@
   +  /**
   +   * Returns the number of times a node has been viewed.
   +   *
   +   * @param strign $order
   +   *   The column name to order by.
   +   *
   +   * @return array
   +   *   An ordered array of node ids.
   +   */
   +  public function fetchAll($order = 'totalcount', $limit = 5);
   

   Typo on "strign".

   Also, the "column to order by" is not descriptive. How do I know what the columns are? Column is an SQL-ism.

   Instead, say the counter to order by, then list the valid options and what they mean. (That in the SQL implementation they happen to map to a column in a table is a coincidence.) I'd go a step further and then stick an assert() statement in the implementation to verify that $order is one of those allowed values. :-) (I blame AkiTendo.)

5. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,90 @@
   +  /**
   +   * Returns if reset is needed.
   +   *
   +   * @return bool
   +   *  TRUE if reset is needed.
   +   */
   +  public function needsReset();
   

   The docblock should explain what reset is and why it would be needed.

1. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -193,19 +193,31 @@ public function build() {
   -      $items [] = \Drupal::l($nodes[$count]->getTitle(), $nodes[$count]->urlInfo('canonical'));
   +      // $items [] = \Drupal::l($nodes[$count]->getTitle(), $nodes[$count]->urlInfo('canonical'));
   +      $items [] = array(
   +        '#type' => 'link',
   +        '#title' => $nodes[$count]->getTitle(),
   +        '#url' => $nodes[$count]->urlInfo('canonical'),
   +        '#cache' => array(
   +          'context' => $nodes[$count]->getCacheContexts(),
   +          'tags' => $nodes[$count]->getCacheTags(),
   +        ),
   +      );
   

   Forgot to remove the commented out line. :-)

2. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -193,19 +193,31 @@ public function build() {
   -      '#cache' => ['tags' => Cache::mergeTags(['node_list'], Cache::buildTags('node', $counts))]
   +      '#cache' => array(
   +          'context' => 'user.roles',
   +          'tags' => $this->entityManager->getDefinition('node')->getListCacheTags(),
   +        ),
          );
   

   I've no idea of this is correct or not. :-)

3. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -71,10 +71,13 @@ public function fetchViews($nid) {
   +    if (in_array($order, array('totalcount', 'daycount', 'timestamp'))) {
   +      return $this->connection->select('node_counter', 'nc')
   +        ->fields('nc', array('nid'))
   +        ->orderBy($order, 'DESC')->range(0, $limit)
   +        ->execute()->fetchCol();
   +    }
   +    return FALSE;
   

   Regarding returning false from a method: http://www.garfieldtech.com/blog/empty-return-values

   If we're allowed to use assertions, that would be ideal. If not, throw \InvalidArgumentException.

4. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -64,7 +67,10 @@ public function fetchAll($order = 'totalcount', $limit = 5);
      /**
   -   * Returns if reset is needed.
   +   * A reset is performed daily.
   +   * Returns if 24 hours has passed since the last reset.
   +   *
   +   * @see StatisticsStorageInterface::resetDayCount()  To perform the reset.
   

   Needs linebreak between first line and description. Plus the first line should be properly structured. Suggested:

   "Returns true if it's been more than 24 hours since daily totals have been reset."

Hey, it's Drupal's coding standard, not mine. :-) Looks good to me, modulo verifying the cache context info. RTBC aside from that. Nice work, Tim!

1. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +176,49 @@ public function build() {
    
   +  protected function nodeTitleList($counts, $title) {
   

   Missing doxygen.

2. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +176,49 @@ public function build() {
   +      '#cache' => array(
   +          'context' => 'user.roles',
   +          'tags' => $this->entityManager->getDefinition('node')->getListCacheTags(),
   +        ),
   

   Should this be per user.role as in the role is different or is this permissions based?

   In that case should use user.permissions.

   --

   If we add the node list tag here, we could remove the tags from above as that one will override the other ones.

   Do we really need the node list tags here?

   Overall +1 to add cacheable metadata though :).

Aaaaand scene. :-)

+++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
@@ -193,6 +193,18 @@ public function build() {
+   * The title for the list. ¶

nit nit nit nit - superfluous space at end of line, can be fixed at commit time.

RTBC + 1

1. This issue is a task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary. That said, in my mind I can't see how this issue meets the criteria for inclusion in the beta. It looks like a 8.1 change to me - which makes me sad because of all the work that has gone on here already. The change makes lots of sense and is architecturally sound.

2. +++ b/core/modules/statistics/statistics.module
   @@ -68,79 +68,22 @@ function statistics_node_links_alter(array &$node_links, NodeInterface $entity,
   + * @deprecated in Drupal 8.0.x, use fetchViews in statistics_storage service.
   

   The format for a deprecation is like this:

    * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.
    *   Use \Drupal::service('email.validator')->isValid().
   

3. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +176,60 @@ public function build() {
   +  /**
   +   * Generates the render array for the block.
   +   *
   +   * @param array $counts
   +   * An ordered array of node ids.
   +   *
   +   * @param string $title
   +   * The title for the list. ¶
   +   *
   +   * @return array
   +   *  A render array for the list.
   +   */
   

   Needs to be indented and have space from end of line removed.

4. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,123 @@
   +  * Construct the statistics storage.
   

   Constructs

5. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,123 @@
   +        'timestamp' => REQUEST_TIME,
   ...
   +    return (REQUEST_TIME - $statistics_timestamp) >= 86400;
   ...
   +    $this->state->set('statistics.day_timestamp', REQUEST_TIME);
   

   Inject the request stack to get the request time. That makes this more unit testable. You could a have a protected method to get the request time from the current request.

6. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,100 @@
   + * Provides an interface defining Statistics Storage
   + * Stores the views per day, total views and timestamp of last view
   

   Missing full stop at end of first line and needs a blank line after the first one.

7. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,100 @@
   +   * The number of node ids to return.
   

   Needs indenting. Plus IDs not ids.

8. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,100 @@
   +   *   An ordered array of node ids.
   

   The patch needs a general search for id/ids in documentation and replacing with ID/IDs.

9. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,100 @@
   +  /**
   +   * Delete counts for a specific node.
   +   *
   +   * @param int $nid
   +   *   The id of the node which views to delete.
   +   *
   +   * @return bool
   +   *   TRUE if the node views have been deleted.
   +   */
   +  public function clean($nid);
   

   Clean is an odd method name for deleting a specific node - how about deleteNode() and accept a node entity instead of just the nid - maybe an alternate storage would want to key by uuid. Hmmm I see all the other methods are accepting an nid. So leave it at that but this method should have a better name.

10. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
    @@ -0,0 +1,100 @@
    +   * Returns true if more than 24 hours since daily totals have been reset.
    

    The 24 hours sounds like an implementation detail and not something that should be on the interface.

After discussing with @xjm and given #103.1 and the fact I can't see how this meets the criteria I'm going to postpone now. Since actioning the other feedback in #103 is taking resources away from fixing what we have to fix to get D8 out of the door.

I'm sorry @timmillwood I wish we'd done the beta evaluation earlier.

Found additional code style problems on top of those reported by Alex. And more severe problems, but nothing truly severe :)

1. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +176,60 @@ public function build() {
   +    $nodes = $this->entityManager->getStorage('node')->loadMultiple($counts);
   

   Shouldn't this also be doing getTranslationFromContext(), to ensure you get the right translation?

   Suggests missing test coverage.

2. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +176,60 @@ public function build() {
   +    $items = array();
   ...
   +    return array(
   

   We use the short array notation for new code.

3. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +176,60 @@ public function build() {
   +      $items [] = array(
   

   Should not be a space between $items and [].

4. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +176,60 @@ public function build() {
   +        '#cache' => array(
   +          'context' => $nodes[$count]->getCacheContexts(),
   +          'tags' => $nodes[$count]->getCacheTags(),
   +        ),
   

   This looks correct; but I'd still like to see at least one integration test for it.

   You can use assertCacheTags() and assertCacheContexts(); they make it easy to check this :)

   (This is what you pinged me about for a review on Twitter — sorry for the delay!)

5. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -124,21 +176,60 @@ public function build() {
   +      '#cache' => array(
   +          'context' => 'user.permissions',
   +          'tags' => $this->entityManager->getDefinition('node')->getListCacheTags(),
   +        ),
   +      );
   

   Strange indentation here.

(because planning to make this work with all entities as soon as this patch is committed)

View statistics for all entity types? :)

Fixing a trailing whitespace. I also wanted to see if we could reconsider adding this in 8.0.x. It will make migration much more simpler. I marked #2500521: Upgrade path for Statistics 7.x and 6.x as postponed on this issue. @alexpott, thoughts? I will work on a beta evaluation if you think it makes sense.

I noticed that there was already a beta evaluation and updated it. I am going out on a limb and marking this as unfrozen as it helps migration (which is unfrozen). If that is not correct, please feel free to remove the unfrozen clause.

1. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -121,24 +173,64 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   +   * Generates the render array for the block.
   

   This comment is wrong, because it doesn't generate the render array for the entire block, it is merely a helper method for build(), which is the method that would fit this description.

2. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -121,24 +173,64 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   +   * @param array $counts
   +   *   An ordered array of node ids.
   +   *
   +   * @param string $title
   +   *   The title for the list.
   

   The \n between these shouldn't be there.

3. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -121,24 +173,64 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   +  protected function nodeTitleList($counts, $title) {
   ...
   +    foreach ($counts as $count) {
   

   $counts feels like a strange variable name here.

4. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -121,24 +173,64 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   +        'context' => 'user.permissions',
   

   First off, it's 'contexts', and it needs to be an array.

   Second: where does the permission checking happen? I.e. I don't think we actually want this cache context? Perhaps node access grants are being checked somehow (I don't see where though?), and that's why? We have the user.node_grants:view cache contexts for that though. See https://www.drupal.org/developing/api/8/cache/contexts.

5. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,142 @@
   + * Provides the default database storage backend for statistics module.
   

   Nit: s/module//

6. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,142 @@
   +  /**
   +  * Constructs the statistics storage.
   +  *
   +  * @param \Drupal\Core\Database\Connection $connection
   +  *   The database connection for the node view storage.
   +  * @param \Drupal\Core\State\StateInterface $state
   +  *   The state service.
   +  */
   ...
   +  /**
   +  * {@inheritdoc}
   +  */
   ...
   +  /**
   +  * {@inheritdoc}
   +  */
   

   Indentation errors in these and other docblocks.

7. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,142 @@
   +      ->condition('nid', $nid, '=')->execute()->fetchAssoc();
   ...
   +      ->orderBy($order, 'DESC')->range(0, $limit)
   +      ->execute()->fetchCol();
   

   Don't method chain calls belong on new lines?

8. +++ b/core/modules/statistics/src/Tests/StatisticsReportsTest.php
   @@ -49,9 +54,16 @@ function testPopularContentBlock() {
   +    $this->assertCacheTags(array_merge(
   

   Please use Cache::mergeTags() instead of array_merge().

+++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
@@ -194,29 +194,28 @@ public function build() {
    * @param array $counts

This is not yet updated.

RE: #14.8: In other tests, we then just call Cache::mergeTags() several times. Yes, annoying, yes, ugly, but that's because we chose to micro-optimize the performance of Cache::mergeTags() for the most common case.

Unfrozen because it helps with migration for node counters.

How exactly does it do this?

@alexpott: Right now, there is no clean reusable way to write to node_counter table. The statistics module just directly runs queries or calls db_merge() to write. I started with doing the same in the migration destination handler and then realized that a storage service would be better. I then found this issue which does the same thing. See #2500521-4: Upgrade path for Statistics 7.x and 6.x for my comment there and the partial patch I have uploaded.

Rerolled.

@hussainweb well we have a working d6 migration without this.

As promised in #2543554: Clean up Help & Statistics blocks — which broke this — helping to resolve conflicts.

(No credit please.)

@alexpott: The D6 migrations do not migrate the statistics themselves (which are stored in node_counter table).

I did a test migration from D6 to D8 just now and confirmed that the settings for Statistics module is migrated (which is d6_statistics_settings.yml), but not the data inside node_counter table itself. In fact, I did a grep on whole migrate code base and didn't find anything that would migrate data in node_counter table.

This blocks the migration path for Statistics 7.x to 8.x: #2500521: Upgrade path for Statistics 7.x and 6.x

Re-rolled https://www.drupal.org/node/1446932#comment-10193353

Here's a quick review for you. Mostly nits. Because mostly looking good already :)

1. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -20,17 +25,64 @@
   +  protected $entityManager;
   

   Since https://www.drupal.org/node/2549139, we shouldn't inject the entity manager anymore.

2. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -84,31 +136,69 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   +   * @param array $nids
   

   So is this int[]?

3. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -84,31 +136,69 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   +  protected function nodeTitleList($nids, $title) {
   

   We can typehint: array $nids

4. +++ b/core/modules/statistics/src/Plugin/Block/StatisticsPopularBlock.php
   @@ -84,31 +136,69 @@ public function blockSubmit($form, FormStateInterface $form_state) {
   +        '#cache' => [
   +          'contexts' => $node->getCacheContexts(),
   +          'tags' => $node->getCacheTags(),
   +        ],
   +      ];
   

   Better yet would be to use \Drupal\Core\Render\RendererInterface::addCacheableDependency(), then any entities that have max-age set will also have that work, and less manual code here.

5. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,101 @@
   + * Stores the views per day, total views and timestamp of last view
   + * for all nodes on the site.
   

   80 cols.

6. +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
   @@ -0,0 +1,101 @@
   +  /**
   +   * Returns the number of times a node has been viewed.
   +   *
   +   * @param int $nid
   +   *   The ID of the node to fetch the views for.
   +   *
   +   * @return array
   +   *   An associative array containing:
   +   *   - totalcount: Integer for the total number of times the node has been
   +   *     viewed.
   +   *   - daycount: Integer for the total number of times the node has been viewed
   +   *     "today". For the daycount to be reset, cron must be enabled.
   +   *   - timestamp: Integer for the timestamp of when the node was last viewed.
   +   */
   +  public function fetchViews($nid);
   +
   +  /**
   +   * Returns the number of times a node has been viewed.
   +   *
   +   * @param string $order
   +   *   The counter name to order by:
   +   *   - 'totalcount' The total number of views.
   +   *   - 'daycount' The number of views today.
   +   *   - 'timestamp' The unix timestamp of the last view.
   +   *
   +   * @param int $limit
   +   *   The number of node IDs to return.
   +   *
   +   * @return array
   +   *   An ordered array of node IDs.
   +   */
   +  public function fetchAll($order = 'totalcount', $limit = 5);
   

   Is it allowed to return other things besides totalcount, daycount and timestamp?

   If not, then shouldn't we introduce a value object rather than returning such arrays? That'd enforce alternative implementations to not return additional things.

7. +++ b/core/modules/statistics/src/Tests/StatisticsReportsTest.php
   @@ -49,9 +54,22 @@ function testPopularContentBlock() {
   +    $this->assertCacheTags(
   +      Cache::mergeTags(
   +        Cache::mergeTags(
   +          Cache::mergeTags(
   +            $node->getCacheTags(),
   +            $block->getCacheTags()
   +          ),
   +          $this->blockingUser->getCacheTags()
   +        ),
   +        ['block_view', 'config:block_list', 'node_list', 'rendered', 'user_view']
   +      )
   +    );
   

   Eh… wow :P

   What about:

   $tags = Cache::mergeTags($node->getCacheTags(), $block->getCacheTags());
   $tags = Cache::mergeTags($tags, $this->blockingUser->getCacheTags());
   $tags = Cache::mergeTags($tags, ['block_view', 'config:block_list', 'node_list', 'rendered', 'user_view']);
   $this->assertCacheTags($tags);
   

8. +++ b/core/modules/statistics/statistics.services.yml
   @@ -0,0 +1,6 @@
   +  statistics.statistics_storage:
   +    class: Drupal\statistics\StatisticsDatabaseStorage
   

   Why not just statistics.storage?

#139 addressed everything in #137 except points 4 and 6.

#141: interdiff is missing.

Here's an interdiff between 139 and 141.