Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
As a relic of Drupal <6 the Statistics module only counts node views. Now we have many different entities we should be counting those too.
Proposed resolution
- Replace the node_counter table with statistics_counter
- Add column for entity type
- Replace node id column with entity id
- Replace statistics_node_view with statistics_entity_view
- Replace statistics_node_predelete with statistics_entity_predelete
Remaining tasks
All of them
User interface changes
none
API changes
none
Data model changes
- Table name changes
- Column name changes
Comment | File | Size | Author |
---|---|---|---|
#101 | 11.x_2532334_101.patch | 69.49 KB | thtas |
#101 | 10.2.x_2532334_101.patch | 69.69 KB | thtas |
#101 | 10.1.x_2532334_101.patch | 69.3 KB | thtas |
#96 | 2532334_96.patch | 67.58 KB | joey91133 |
#91 | 2532334_91.patch | 67.58 KB | joey91133 |
Comments
Comment #1
timmillwoodWaiting on #1446932: Improve statistics performance by adding a swappable backend
Comment #3
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #4
mallezieLet's really postpone it on that one.
Comment #5
mallezieComment #7
timmillwoodComment #8
jibranI had brief chat with @timmillwood. That's how I think we should change this:
Comment #9
timmillwoodWe won't be able to replace statistics_node_view and statistics_node_predelete, we would have to support both node and entity versions, but maybe deprecate the node versions.
Comment #10
jibranNodeStatisticsDatabaseStorage
was only introduced in 8.3.x so I changed the whole file and made it generic.statistics.php
ajax call I movedresetDayCount
to new service.StatisticsStorageInterface::fetchViews()
so I removed it.StatisticsStorageInterface::fetchViews()
thenStatisticsRedisStorage
can implement a protected method but I think module needs a re-write anyway. :DComment #11
jibranForgot to add
core/modules/statistics/src/Plugin/views/field/EntityCounterTimestamp.php
Comment #12
benjy CreditAttribution: benjy at PreviousNext commentedInitial review, didn't actually test it out.
You pass the $entity_type_id in here both the $entity_type is most other places? Maybe we should make it consistent?
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.
I wonder if we need to clearly state on the form that un-checking will delete all existing data? Maybe even a confirmation page?
Do we need to handle cleaning up the tables here now?
Saves twice.
No reason not to use === in each of these.
The brackets aren't needed inside the quotes?
Reset isn't per entity type?
Any reason not todo that here?
===
Un-related change?
I can't see this class in the patch?
Comment #13
tstoecklerNote that the entity ID may be a string.
Comment #14
jibranThanks @benjy for the review here is my response.
@tstoeckler Yeah, I thought about it but then procrastination won. Fixed it.
Comment #16
timmillwoodRe-roll of #14.
I don't think it's possible to remove NodeStatisticsDatabaseStorage in a BC way, but maybe we can deprecate it some how and run NodeStatisticsDatabaseStorage and the more generic StatisticsDatabaseStorage together.
Comment #18
timmillwoodProgress.
Needs some stuff to make it backwards compatible.
Comment #20
timmillwoodOoops typo.
Comment #22
jibranThanks 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.Comment #23
timmillwood@jibran - I was wondering if we could keep the interface as it was before, but with
$entity_type = null
so it'll work for the old service and the new service. The old service will then just extend the new service and pass in the Node entity type. If that doesn't work we'll just have to have two interfaces, but can still have the old service extend the new one.statistics.php will always call the new service, we just need to keep the old one around for backwards compatibility.
Thinking a factory might be overkill.
Started work last night added all deleted test and the old service back in. Will try to finish that off tonight.
Comment #24
jibranAwesome. Here is an interdiff for PHPCS fails.
Comment #25
timmillwoodGreat stuff, thanks!
Comment #26
timmillwoodThis will fail testing, but shows progress.
Comment #28
timmillwoodTrying to fix some of the failing tests, and rebase after the tests moved.
Comment #30
jibranI don't think this is a correct fix.
Comment #31
timmillwood@jibran - yes, should've added a @todo to that, wasn't really sure what was going on.
I think many of the remaining test fails are relating to the *_counter table now being lazy loaded.
Comment #32
timmillwoodComment #34
timmillwoodComment #35
jibranNice work! Now we add tests.
Comment #36
dawehnerI'm wondering whether it makes sense to add a constant to ensure that the value in here is an actual entity type.
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.
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.
Comment #39
Wim LeersI just ran into #103866: Add a general counter API, which seems an even more generic predecessor to this :)
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 theirEntityViewController
s/canonical pages?Why a specific table per entity type?
Why not one
counter
table withentity_type_id
as a column? Or perhaps even just trackinguuid
.Comment #40
timmillwoodRe: #39.2 - there were a few thoughts behind having a table per entity type.
Comment #41
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #42
Wim Leers#40: RE #39.2:
Comment #43
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commentedre-rolled
Comment #44
timmillwood@Wim Leers - Honestly, I'm pretty ambivalent.
I guess we could look at converting the
node_counter
table tostatistics
and add an entity_type_id column?Comment #49
astringer CreditAttribution: astringer commentedI also tried to apply the patch through Composer and it failed.
Comment #50
isabelle.wagenvoord CreditAttribution: isabelle.wagenvoord as a volunteer commentedthis 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.
Comment #51
isabelle.wagenvoord CreditAttribution: isabelle.wagenvoord as a volunteer commentedHere is my second attempt at rerolling the patch. I fixed the syntax error I introduced.
Comment #54
gantal CreditAttribution: gantal at Elevated Third commentedTagging for DrupalCamp Colorado's upcoming contrib day.
Comment #55
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedRe-roll patch created for 9.1.x.
Comment #57
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedFixed CS issue and created service 'statistics.storage.node'
Comment #60
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commentedWorked 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.
Comment #62
sanjayk CreditAttribution: sanjayk as a volunteer and at Material for Drupal India Association commented@here kindly review the patch and let me know if any changes required.
Comment #63
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedRe-roll patch created for 9.2.x.
Comment #65
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedFixing fail tests.
Comment #66
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedRe-rolled #65.
Comment #68
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedFixing 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.
Comment #70
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedFixing the test.
Comment #72
robphillips CreditAttribution: robphillips commentedRerolled #70 for 9.1.x.
Comment #74
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedSome files are missing in re-roll provided in #72.
Comment #75
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedRe-roll patch created for 9.3.x, Please have a look.
Comment #76
robphillips CreditAttribution: robphillips commentedRerolled #72 with missing changes for 9.1.x.
Comment #78
robphillips CreditAttribution: robphillips commentedWrong file uploaded.
Comment #79
robphillips CreditAttribution: robphillips commentedComment #80
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedFixed the custom command fail issue for 9.3.x.
Comment #81
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedPlease ignore my previous comment #80.
Fixed the custom command fail issue for 9.3.x.
Comment #83
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedFixed the fail tests in #81. Please have a look.
Comment #85
robphillips CreditAttribution: robphillips commentedRe-rolled #70 for latest 9.2.x. Unable to apply #70 to a fresh 9.2.4 install.
Comment #86
robphillips CreditAttribution: robphillips commentedFixes missing parameter definition in #85.
Comment #87
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedFixed the fail test in #83 for 9.3.x, Please have a look.
Comment #89
joey91133 CreditAttribution: joey91133 as a volunteer commentedFixed the fail test in #83 for 9.3.0 release.
Comment #90
joey91133 CreditAttribution: joey91133 as a volunteer commentedFixed the fail test in #83 for 9.3.0 release.
Comment #91
joey91133 CreditAttribution: joey91133 as a volunteer commentedFixed the fail test in #83 for 9.3.0 release. (fix apply failed)
Comment #93
b_sharpe CreditAttribution: b_sharpe at ImageX commentedFYI: 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.
Comment #96
joey91133 CreditAttribution: joey91133 as a volunteer commentedFixed the fail test in #83 for 9.5.x release.
Comment #97
quietone CreditAttribution: quietone at PreviousNext commentedStatistics 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.
Comment #99
thtas CreditAttribution: thtas commentedAttached is a re-roll against 10.1.x, 10.2.x and 11.x
Comment #100
thtas CreditAttribution: thtas commentedComment #101
thtas CreditAttribution: thtas commented