Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
it would be helpful to have a "clear all recorded statistics" button.
Steps to reproduce
NA
Proposed resolution
Add a "Reset" button to the statistics admin page.
Remaining tasks
Review from committers
User interface changes
API changes
NA
Data model changes
NA
Release notes snippet
NA
Original post
Hi,
I think it would be helpful to have a "clear all recorded statistics" button. For example, this would be useful for when a site transitions from being a private "development" site to being a public "live" site.
Comment | File | Size | Author |
---|---|---|---|
#64 | Screen Shot 2022-12-30 at 10.46.05 AM.png | 87.44 KB | smustgrave |
#60 | 54798-60.patch | 4 KB | pradhumanjain2311 |
|
Comments
Comment #1
LAsan CreditAttribution: LAsan commentedBump.
Comment #2
Andrew Schulman CreditAttribution: Andrew Schulman commentedSubscribing. I agree, this is a pretty basic need and common use case. Thanks, Andrew.
Comment #3
j.somers CreditAttribution: j.somers commentedAttached is a small patch which adds a button to the statistics administration page similar to the "Clear cache" button on the performance administration page. The description and title information could possibly be described better.
Comment #4
Dave ReidI'd include the node_counter table as well since it is a part of the statistics.module.
Comment #5
j.somers CreditAttribution: j.somers commentedI added it in the attached patch, but note that the node_counter table is not created by the statistics module.
Comment #6
j.somers CreditAttribution: j.somers commentedDiscard the previous patch, I added a test case in the attached one.
Comment #8
j.somers CreditAttribution: j.somers commentedDon't really see why applying the patch would have failed but after #463064: 'content' cannot be used as a $form element and breaks any page which uses it, such as admin/settings/statistics has been resolved I shall create a new version.
Comment #9
j.somers CreditAttribution: j.somers commentedRe-roll.
Comment #10
catchThis isn't need for setUp().
The delete from table should use db_truncate()->execute().
Comment #11
j.somers CreditAttribution: j.somers commentedUpdated the patch to make sure it works on the latest D7 version and applied remarks in comment #10.
Comment #12
MichaelCole CreditAttribution: MichaelCole commented#11: jsomers_54798_5-D7.patch queued for re-testing.
Comment #13
ghills88 CreditAttribution: ghills88 commentedThe patch from #11 works well for me.
I applied it to the CVS HEAD and the patch applied correctly. The button was indeed added to the statistics admin area and clicking reset had the desired behavior of resetting the site hit statistics.
Comment #14
j.somers CreditAttribution: j.somers commentedMarking as RTBC because of comment #13.
Comment #15
webchickSorry, we're past feature freeze (and string freeze) for D7; new features go in the next release of Drupal.
Comment #16
TR CreditAttribution: TR commentedIt's been a year and a half since the patch, so I re-rolled it against the D8 head. The only changes I made from the patch in #11 were to remove trailing whitespace, add doxygen comments, and modify some doxygen comments to bring up to standards.
I tested this patch in Drupal 8.x and it works as advertised.
This issue has been open for 5 years! I'm reluctantly changing the status to needs review to force the testbot to test the patch, but I'd like to think this is RTBC. Maybe someone else can do a quick review and we can finally get this closed.
Comment #17
TR CreditAttribution: TR commentedIt would help if I actually attached the patch ...
Comment #19
TR CreditAttribution: TR commented#17: 54798-clear-statistics.patch queued for re-testing.
Previous test failure seems like it was a testbot problem: "PHP Fatal error: Class 'DrupalWebTestCase' not found"
Comment #21
karschsp CreditAttribution: karschsp commentedHere's a re-roll for 8.x.
Comment #22
karschsp CreditAttribution: karschsp commentedFixing whitespace issues.
Comment #23
naxoc CreditAttribution: naxoc commented#22: 54798.reset-statistics.022.patch queued for re-testing.
Comment #25
naxoc CreditAttribution: naxoc commentedReroll.
I did some cleanup in the test - mainly removing the
t()
around the text in assertions.Comment #26
TR CreditAttribution: TR commentedI don't think removing the t() is appropriate until #500866: [META] remove t() from assert message is committed.
Comment #27
naxoc CreditAttribution: naxoc commented@TR -
t()
should not be added to new tests anymore. See http://drupal.org/node/265828 at the top.Comment #28
TR CreditAttribution: TR commentedThe revision history of the documentation you cite at http://drupal.org/node/265828 says that this documentation was changed because of what is being discussed in #500866: [META] remove t() from assert message. So again, I think it was premature and inappropriate to change the documentation before the community has finished deciding the issue. #500866: [META] remove t() from assert message has been open a long time - I don't think that subverting the community process by making changes in anticipation of the expected outcome of #500866: [META] remove t() from assert message is something that we should be doing.
Comment #29
naxoc CreditAttribution: naxoc commentedCurrent documentation says don't use
t()
: http://drupal.org/simpletest-tutorial-drupal7#tReference found here: http://drupal.org/node/500866#comment-5654154
Comment #30
mgifford25: 54798.reset-statistics.25.patch queued for re-testing.
Comment #37
Wim LeersSite builders advanced enough to have multiple environments (dev/staging/prod or just dev/prod) seem more than capable of just exporting the
node_counter
table as "structure only" when they sync their DB?Comment #38
SKAUGHT'Site builders' may include people who don't have enough database and module understanding, or access to a database UI/term to truncate.
Comment #39
TR CreditAttribution: TR commented@Wim Leers:
Core has a UI button for deleting DB logs, clearing caches, cleaning up testing results, re-indexing the site for search purposes, etc. All these things can be done from the command-line or outside the UI by a knowledgeable developer. I think the statistics node counts are similar to these things, and they need to be cleared during site development for similar purposes. Testing content, theming content, testing a site, all generate a lot of bogus data that you don't want polluting your site when it goes live.
I think it would be a shame if Drupal stopped supporting non-developers, because if that's case much of the core UI is unneeded. (What developer enables modules via the UI these days? What developer manages config through the UI? Etc.) We should make it possible to manage the data generated by the core statistics module through the UI, just like we make it possible to manage the dblog data, simpletest data, search data, and caches.
Comment #40
TR CreditAttribution: TR commentedComment #43
ressa CreditAttribution: ressa at Ardea commentedHere is a re-roll of the patch for Drupal 9, which seems to work. I removed the
accesslog
bits in the test, since this part was removed in #1446956: Remove the accesslog from statistics, but the test probably needs more work to pass.Comment #45
ressa CreditAttribution: ressa at Ardea commentedUpdates failing test method to
public
.Comment #47
xjmThanks for the suggestion!
Feature requests go against the 9.1.x branch at this point. Both D7 and D8 are going into LTS so we would not backport a new feature.
Comment #48
ressa CreditAttribution: ressa at Ardea commentedThanks for the update! I am trying to get PHPUnit tests running in Lando, and if I succeed I'll try to look at the failing tests.
Comment #49
TR CreditAttribution: TR commentedRe-rolled for Drupal 9 and to fix the test failures in #45. I injected the database connection and modified the patch to use current coding standards. See interdiff.
Comment #51
TR CreditAttribution: TR commentedI missed converting a drupalPost() to drupalPostForm() ...
Comment #52
andypostLooks rtbc, but needs is update and release notes snippet
Comment #55
TR CreditAttribution: TR commentedRe-roll for 9.3.x.
Comment #59
nod_D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.
Also I would expect a confirmation step before removing all the statistics from a site.
Comment #60
pradhumanjain2311 CreditAttribution: pradhumanjain2311 at OpenSense Labs for DrupalFit commentedTry to make patch #55 drupal version 10.1.x compatible.
Comment #61
TR CreditAttribution: TR commentedYou know, I first contributed to this issue only 11.5 years ago, but it has been open for more than 16 years. How many people even remember what Drupal version "x.y.z" meant? That's what this was originally opened against.
Because Drupal core is constantly changing, there will constantly be new things that this patch "needs" to change. My expectation has been that if I do the work and address the feedback that raised, then the patch will be committed in a reasonable amount of time, before core can drastically change underneath it. That hasn't been the case.
If this had been committed 10 years ago, then the code would have evolved along with the rest of core. But constantly delaying and constantly kicking this issue down the road to D7, D8, D9, and now D10 just means it will never be up to current standards.
And now we have yet another new request - that we have a confirm form for this. If I make this change do I have any assurance that my work won't go to waste?
There are a lot of people who are willing to contribute to core, but constantly disrespecting their efforts by ignoring and bikeshedding issues like this has frankly been killing the community over the years.
If there is no intention of committing this, then just say so. You could have said so 10 years ago and not wasted our time. Mark the issue as "won't fix" and move on. It's just plain disrespectful to leave it open and constantly nit pick for years without movement or feedback from the maintainers.
Comment #62
TR CreditAttribution: TR commentedComment #63
nod_I understand the feeling, a few of my long standing patches are in the same situation so I know what it feels like.
For some background the Needs review queue is at 2600+ issues today. It's nowhere near manageable, after some looking into I found that 65% of those need review issues have patches that either don't apply or don't pass commit checks. So I'm trying something to address this and make the Needs review queue a more manageable size so that patches can be worked on, reviewed and committed in a timely manner. Nobody wants to ignore a patch, it's just that there are too many open to review.
Now back to the topic at hand from #39:
Today the UI for deleting DB logs looks like a confirm form, the button is not in the main watchdog interface but in a separate tab. This was changed in #2506449: Transform "Clear log messages" submit button into a link in admin/reports/dblog (2016) and I would think that for consistency we go with the same pattern. But pradhumanjainOSL rerolled the patch, it's green, you can definitely RTBC it if you feel it's ready.
It's not up to me to say whether something makes it in core, it's possible to RTBC and have feedback from a core committer. They always have the final say.
And to answer the question about a commit guarantee. I can't tell you that, I don't have commit access to this core repository. I had some success making old patches go through this past week since I started going through the NR list, so it's possible.
Comment #64
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
Fully understand the frustration mentioned here which is why we have started this new initiative to help get the "needs review" queue under control so that issues don't sit forever. This won't necessarily get anything fixed faster but users will know sooner more is needed.
I've went ahead and updated the IS so removing the tag for that.
Testing the patch #60 and can confirm it is doing as expected.
Testing on Drupal 10.1.x
Enabled statistics
Added a view column to the admin/content view
Visited a few pages, Verified count is updated.
Reset the statistic
Verified count is now 0
Comment #65
xjmIn all this issue's years, it doesn't look like it's ever gotten a usability review. :)
You can get usability feedback in the
#ux
channel in Drupal Slack. There are weekly meetings Fridays at 1400 UTC where you can join a Zoom and have your user interface change reviewed for usability and accessibility by a team of core topic maintainers and volunteers. Thanks!Comment #66
xjmAlso, it's not a bug. :)
Comment #67
xjmComment #68
xjmUsability review
We discussed this issue at #3329009: Drupal Usability Meeting 2023-01-06. That issue will have a link to a recording of the meeting.
For the record, the attendees at today's usability meeting were @ABElliott, @BlackBamboo, @benjifisher, @rkoller, @shaal, and @xjm.
Points we covered in the discussion:
We all agreed that there should be a confirmation form for this action, since it is a destructive action.
We discussed the organization of the form. The current structure, with the "Reset" button as an immediate action in between the rest of the form and the submit button, is confusing. We discussed two options to improve it:
We agreed that this issue is not passing the documentation gate currently. We need a couple sentences of documentation added to
hook_help()
and to Help Topics about resetting statistics.We agreed that there also needs to be some explanation of what "Reset statistics" means in the context of the form itself. The best suggestion we had was that the form should tell you the specific consequences of the action, e.g.:
(That's just a starting point. The text could probably be improved.)
Then, on the confirmation form.
And finally in the confirmation message:
If there are no records in the database table, the text should be something else, maybe simply:
(Or something along those lines.)
Currently, the button is shown regardless of whether there are any statistics, or whether statistics are even enabled. @xjm suggested that maybe the button should only conditionally be shown if "Count content views" is enabled; however, @benjifisher pointed out that a site owner could enable statistics, then disable it, and there would still be database records that could be purged. So, it should not be hidden conditionally based on that. The previous point (displaying the number of statistics records and the timeframe, or that there are none) would be a better way of addressing this.
@xjm also noticed in the course of manually testing the patch that caches are not properly invalidated when the reset button is used. For example, the block cache for the statistics blocks is not invalidated. This makes it appear that nothing is happening when the user first clicks the reset button. The cache for any content containing statistics data should be invalidated when the form is submitted. The cache invalidation aspect also needs test coverage.
Code review
Currently, the patch is directly truncating a database table:
This would have been the correct approach in 2006, but it no longer is. Instead of using the database connection directly, the form should use
statistics.storage.node
. (This is probably the key to fixing the cache invalidation bug mentioned above.)Also, instead of hardocoding a query, we should use StatisticsStorageInterface. There is currently a deleteViews() method that delete the records for a single entity. To match that, we could add a new method, something like
deleteAllViews()
.Comment #74
xjmAdding issue credit for Usability meeting participants.
Comment #75
ABElliott CreditAttribution: ABElliott at Agileana commentedTook the following steps to test the site and had no issues:
Go to https://simplytest.me/configure?project=drupal&version=10.1.x-dev
On https://www.drupal.org/project/drupal/issues/54798, right-click the filename '54798-60.patch” and select “Copy link address” (or similar). This is the URL to the patch file.
On https://simplytest.me/configure?project=drupal&version=10.1.x-dev, under “Advanced options”, paste the patch file URL in the field labeled “Add patches on the chosen project”.
Click “Add patch”.
Click the “Launch Sandbox” button. It will build an instance of Drupal for you with the patch applied. This can take a long time (up to 5 minutes).
Log into the new Drupal site using username and password admin/admin.
At /admin/modules on your test site, check the box next to the Statistics module and click “Install”.
At /admin/config/system/statistics, check “Count content views”.
At /admin/people/permissions/module/statistics, grant all roles permissions to view content hits.
Create at least three pieces of test content at node/add/article.
On /admin/structure/block, click the “Place block” button next to “Hero” and select “Popular content”. Set “Number of all time views to display” to 3 and click “Save block”.
Click “Home” and observe that one of your test articles is now linked in the upper left.
View each piece of content several times.
Go to /admin/config/development/performance and click “Clear all caches”.
Click “Back to site” and observe your block listing the popular content, ordered by view.
Go back to /admin/config/system/statistics and click “Reset”.
Go to /admin/config/development/performance and click “Clear all caches”.
Click “Back to site” and observe that the popular content listing is now empty.
Comment #76
ABElliott CreditAttribution: ABElliott at Agileana commentedFixing attribution
Comment #78
AnybodyStill needs #68 to be implemented by someone. Would be a nice addition!
Comment #79
2dareis2do CreditAttribution: 2dareis2do commentedLooks like statistics module is being removed from 11.x unfortunately.
https://www.drupal.org/project/ideas/issues/3266457
Comment #80
TR CreditAttribution: TR commentedYeah, typical. Drupal core loses a feature because none of the core committers noticed the regression. But then when it is pointed out, there is push back to fixing it. In this case, the fix has been delayed for EIGHTEEN YEARS. Then the submodule gets removed from core, so there's no need to fix it. Thanks for wasting my time. No one remembers any more that we used to have this feature.
When I contributed patches for this issue, they were 100% compliant with the then-current Drupal standards. Those standards keep changing. If the patches had been committed 10 years ago, then this code would have organically evolved along with the rest of Drupal. But these patches keep getting judged by standards that DIDN'T EXIST when they were contributed, and when they are re-rolled they are neglected and rejected again.
If you want to encourage contribution to core, you're doing exactly the OPPOSITE of what you should.
I'm out. Clearly my contributions to this issue are not valued. A less patient person would have given up 10 years ago.
Comment #81
2dareis2do CreditAttribution: 2dareis2do commented@tr I believe the statistics removal has been pushed back because it does not meet the criteria of being installed on less than 5% of sites.
To be fair there are a lot of issues that have built up over the years. I have some sympathy, largely due to the limited resources that seem to make up the 'core' drupal team, which if I understand correctly stands at around 20 people. This is only really possible due in part to the enormous good will of these people who give up their own time to try and move Drupal forward.
Why the core team is limited to just 20 people is beyond me?
That said, it is sad when the core team seem to neglect issues like this. Trying to get this and other features to work as they should take far too long and put developers off using Drupal.
Most businesses cannot function on the premise that an issue might be fixed in 18 years time. Usually they want things yesterday. This goes along way to explaining why many companies are turning their backs on Drupal when it takes so long to get things done and no amount of marketing spend will convince people otherwise.
This regression probably should have been resolved one way or another 18 years ago. IMO this and other issues in a similar state should be prioritised over and above new features such as project browser on that basis.
Thats said webform already has a project browser of sorts. I would like to see webform brought into core before building project browser. Core needs to have a form builder imo.