Count all content entities views in the Statistics module

Let's really postpone it on that one.

I had brief chat with @timmillwood. That's how I think we should change this:

• Add per entity table to store stats.
• Update UI/schema to allow entity types selection.
• Add a new service to create new table(s) for each selected entity type(s).
• Replace statistics_node_view with statistics_entity_view.
• Replace statistics_node_predelete with statistics_entity_predelete.
• Update views integration.
• Update tokens.
• Add some new genric tests.

• Here is a working patch with views integration.
• NodeStatisticsDatabaseStorage was only introduced in 8.3.x so I changed the whole file and made it generic.
• To initiate minimum amount of services during statistics.php ajax call I moved resetDayCount to new service.
• As per #9 I maintained the BC.
• With my current approach, I was unable to accommodate StatisticsStorageInterface::fetchViews() so I removed it.
• @timmillwood if https://www.drupal.org/project/redis_statistics needs StatisticsStorageInterface::fetchViews() then StatisticsRedisStorage can implement a protected method but I think module needs a re-write anyway. :D
• I didn't update any tests hence don't test patch.
• Needless to say we need some generic entity test.
• Patch has an upgrade path so I suppose we need upgrade path tests as well.
• Reviews are welcome but I'm not activily working on this anymore.

Forgot to add core/modules/statistics/src/Plugin/views/field/EntityCounterTimestamp.php

Initial review, didn't actually test it out.

1. +++ b/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,173 @@
   +  public function maxTotalCount($entity_type_id) {
   

   You pass the $entity_type_id in here both the $entity_type is most other places? Maybe we should make it consistent?

2. +++ b/modules/statistics/src/StatisticsSettingsForm.php
   @@ -63,19 +97,23 @@ protected function getEditableConfigNames() {
   +      '#title' => $this->t('Select entities'),
   

   Do we use terms like "entity" in the UI? I note right above we have a reference to "Content entity" as well.

   Maybe we should loop over the entity types and render the label. So we have, "Enable statistics for Content/Block/Media" etc.

3. +++ b/modules/statistics/src/StatisticsSettingsForm.php
   @@ -84,9 +122,19 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        $this->statisticsStorage->dropTable($entity_type);
   

   I wonder if we need to clearly state on the form that un-checking will delete all existing data? Maybe even a confirmation page?

4. +++ b/modules/statistics/statistics.install
   @@ -9,53 +9,13 @@
    function statistics_uninstall() {
   

   Do we need to handle cleaning up the tables here now?

5. +++ b/modules/statistics/statistics.install
   @@ -86,3 +46,14 @@ function statistics_update_8002() {
   +  $config->clear('count_content_views')->save();
   ...
   +  $config->set('entity_type_ids', $value)->save();
   

   Saves twice.

6. +++ b/modules/statistics/statistics.module
   @@ -40,7 +40,43 @@ function statistics_help($route_name, RouteMatchInterface $route_match) {
   +  if ($entity_type_id == 'node') {
   ...
   +  if ($route_match->getRouteName() == $route_name) {
   ...
   +  if (!$entity->isNew() && $view_mode == 'full' && $entity_is_page && empty($entity->in_preview)) {
   

   No reason not to use === in each of these.

7. +++ b/modules/statistics/statistics.module
   @@ -40,7 +40,43 @@ function statistics_help($route_name, RouteMatchInterface $route_match) {
   +  $route_name = "entity.{$entity_type_id}.canonical";
   

   The brackets aren't needed inside the quotes?

8. +++ b/modules/statistics/statistics.module
   @@ -70,10 +106,14 @@ function statistics_node_links_alter(array &$links, NodeInterface $entity, array
   +  \Drupal::service('statistics.reset_count')->resetDayCount();
   

   Reset isn't per entity type?

9. +++ b/modules/statistics/statistics.module
   @@ -139,14 +182,28 @@ function statistics_get($id) {
   +  // @todo Remove this condition once statistics_node_view() is removed.
   

   Any reason not todo that here?

10. +++ b/modules/statistics/statistics.module
    @@ -139,14 +182,28 @@ function statistics_get($id) {
    +  if ($entity->getEntityTypeId() == 'node') {
    

    ===

11. +++ b/modules/statistics/statistics.php
    @@ -11,20 +11,32 @@
    -$kernel = DrupalKernel::createFromRequest(Request::createFromGlobals(), $autoloader, 'prod');
    +$request = Request::createFromGlobals();
    +$kernel = DrupalKernel::createFromRequest($request, $autoloader, 'prod');
    

    Un-related change?

12. +++ b/modules/statistics/statistics.services.yml
    @@ -1,6 +1,12 @@
    +    class: Drupal\statistics\StatisticsResetCount
    

    I can't see this class in the patch?

+++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
@@ -0,0 +1,173 @@
+          $idKey => [
+            'description' => "The {{$entity_type_id}}.$idKey for these statistics.",
+            'type' => 'int',
+            'unsigned' => TRUE,
+            'not null' => TRUE,
+            'default' => 0,
+          ],

Note that the entity ID may be a string.

Thanks @benjy for the review here is my response.

1. Yeah, I wrote it like that but then changed it any reverted back to your suggestion.
2. How about now?
3. Yeah, good idea. I'll add it later.
4. Good catch. Fixed.
5. Fixed.
6. Fixed.
7. Hmm https://3v4l.org/UKWcF. Fixed.
8. It was not but it is a good idea. Updated.
9. See #9.
10. Fixed.
11. We are pushing the same request to the request stack blow.
12. Added now.

@tstoeckler Yeah, I thought about it but then procrastination won. Fixed it.

Thanks for fixing the existing tests.
@timmillwood after spending some time with the patch what do you think about the approach? Do you still think we still need
StatisticsStorageFactory as we talked about it before?
Personally, I'd love to go down that road but I want to load minimum files during statistics.php request.

Awesome. Here is an interdiff for PHPCS fails.

+++ b/core/modules/block/tests/src/Kernel/Migrate/d6/MigrateBlockTest.php
@@ -239,9 +239,9 @@ public function testBlockMigration() {
-      'id' => 'broken',
+      'id' => 'statistics_popular_block',
...
-      'provider' => 'core',
+      'provider' => 'statistics',

I don't think this is a correct fix.

Nice work! Now we add tests.

1. +++ b/core/modules/statistics/config/schema/statistics.schema.yml
   @@ -4,12 +4,15 @@ statistics.settings:
   +    entity_type_ids:
   +      type: sequence
   +      label: 'Entity Type IDs'
   +      sequence:
   +        type: string
   +        label: 'Entity Type ID'
   

   I'm wondering whether it makes sense to add a constant to ensure that the value in here is an actual entity type.

2. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,242 @@
   +  public function recordView($entity_type_id, $key, $id) {
   ...
   +        $entity_type = \Drupal::entityTypeManager()->getDefinition($entity_type_id);
   ...
   +  public function fetchView(EntityTypeInterface $entity_type, $id) {
   ...
   +  public function fetchAll(EntityTypeInterface $entity_type, $order = 'totalcount', $limit = 5) {
   

   It is a bit weird to have an api which is partially using the entity type ID and partially entity type, but meh, maybe its just me, even if you actually load the entity type.

3. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,242 @@
   +  public function createTable(EntityTypeInterface $entity_type) {
   

   It is weird that this method is public. I'd have argued that this is purely an implementation detail. Why can't we do a similar kind of exception handling like for example the statistics module.

I just ran into #103866: Add a general counter API, which seems an even more generic predecessor to this :)

1. +++ b/core/modules/search/src/Tests/SearchRankingTest.php
   @@ -95,11 +95,12 @@ public function testRankings() {
   -    $this->config('statistics.settings')->set('count_content_views', 1)->save();
   +    $this->config('statistics.settings')->set('entity_type_ids', ['node'])->save();
   
   +++ b/core/modules/statistics/config/install/statistics.settings.yml
   @@ -1,2 +1,2 @@
   -count_content_views: 0
   +entity_type_ids: {}
   

   This is going away in #2502313: Installing the Statistics module doesn't do anything, one must also know about some pretty mysterious setting and enable it — that's making this module zero-configuration.

   Why reintroduce configuration?

   Can't we just automatically track all entity types on their full view modes? If not that, on their EntityViewControllers/canonical pages?

2. +++ b/core/modules/statistics/src/StatisticsDatabaseStorage.php
   @@ -0,0 +1,242 @@
   +  protected function tableName(EntityTypeInterface $entity_type) {
   +    $entity_type_id = $entity_type->id();
   +    return $entity_type_id . '_counter';
   +  }
   

   Why a specific table per entity type?

   Why not one counter table with entity_type_id as a column? Or perhaps even just tracking uuid.

#40: RE #39.2:

1. #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration has proven that this does not need to be the case.
2. Is this actually a benefit?
3. Is this actually true?

re-rolled

I also tried to apply the patch through Composer and it failed.

this is my attempt to reroll the patch at comment #34. this is my first time doing it as a part of completing Drupal's core ladder.

Here is my second attempt at rerolling the patch. I fixed the syntax error I introduced.

Tagging for DrupalCamp Colorado's upcoming contrib day.

Re-roll patch created for 9.1.x.

Fixed CS issue and created service 'statistics.storage.node'

Worked on few issues which is related to

TypeError: Argument 1 passed to Drupal\statistics\NodeStatisticsDatabaseStorage::fetchView() must implement interface Drupal\Core\Entity\EntityTypeInterface, string given, called in

Got the below solution

$node_entity = \Drupal::entityTypeManager()->getStorage('node')->load($this->node->id());
$statistics = \Drupal::service('statistics.storage.node')->fetchView($node_entity->getEntityType(), $this->node->id());

I have created an object of entity type and load a node and passed into the called service. Kindly review and let me know if need any changes.

Working on rest of the issues once fixed I will update here.

@here kindly review the patch and let me know if any changes required.

Re-roll patch created for 9.2.x.

Fixing fail tests.

Re-rolled #65.

Fixing the test fails issue.

@anmolgoyal74 Re-rolling is not up to the mark in #66, forgot to remove old "fetchView()" function from the file "NodeStatisticsDatabaseStorage.php", Please have a look.

Fixing the test.

Rerolled #70 for 9.1.x.

Some files are missing in re-roll provided in #72.

Re-roll patch created for 9.3.x, Please have a look.

Rerolled #72 with missing changes for 9.1.x.

Wrong file uploaded.

Fixed the custom command fail issue for 9.3.x.

Please ignore my previous comment #80.

Fixed the custom command fail issue for 9.3.x.

Fixed the fail tests in #81. Please have a look.

Re-rolled #70 for latest 9.2.x. Unable to apply #70 to a fresh 9.2.4 install.

Fixes missing parameter definition in #85.

Fixed the fail test in #83 for 9.3.x, Please have a look.

Fixed the fail test in #83 for 9.3.0 release.

Fixed the fail test in #83 for 9.3.0 release.

Fixed the fail test in #83 for 9.3.0 release. (fix apply failed)

FYI: As an alternative - https://www.drupal.org/project/usage_data

Definitely not production ready, but pluggable and event-based tracking based off core's statistics module so can track all entity types, plus routes, views, etc.

Fixed the fail test in #83 for 9.5.x release.

Statistics is approved for removal. See #3266457: [Policy] Deprecate Statistics module in D10 and move to contrib in D11

This is now Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

It will be moved to a contributed Statistics project once the project is created and the Drupal 11 branch is open.

Attached is a re-roll against 10.1.x, 10.2.x and 11.x

Attached is a re-roll against 10.3.x

Tests are failing for modules/search/tests/src/Functional/SearchRankingTest.php

phpunit ./web/core/modules/search/tests/src/Functional/SearchRankingTest.php
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

E.                                                                  2 / 2 (100%)

Time: 00:10.277, Memory: 8.00 MB

There was 1 error:

1) Drupal\Tests\search\Functional\SearchRankingTest::testRankings
Behat\Mink\Exception\ElementNotFoundException: Select with id|name|label|value "edit-rankings-views-value" not found.

/var/www/html/drupal/web/core/tests/Drupal/Tests/WebAssert.php:234
/var/www/html/drupal/web/core/modules/search/tests/src/Functional/SearchRankingTest.php:130

@jordan.jamous thanks for the reroll. Can you convert the path to a MR, since the old bot no longer runs and we have a working Gitlab CI integration ?

+1 on this request, totally makes sense!

Could someone please create the MR from the patch?

The patch here is doing a LOT.

There's stuff related to:

- #103866: Add a general counter API
- #3064152: Separate `statistics` pseudo field from the general `links` field

Also, there doesn't seem to be any config to determine which entity types get counted in the entity view hook.

I think we probably need to take a slower approach to this, tackling other issues first to generalise node-specific things individually.