Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[plg_system_redirect] update hit counter #27440

Closed
wants to merge 1 commit into from
Closed

[plg_system_redirect] update hit counter #27440

wants to merge 1 commit into from

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Jan 8, 2020

Pull Request for Issue #27437 .

Summary of Changes

Update the hit counter even if there is a publisher new url

Testing Instructions

  • enable the redirects plugin and the collect urls option
  • visit several non-existing urls multiple times
  • com_redirects will now list all of the non-existing urls with a hit count
  • Take one of the non-existing urls and edit it so that it has a new url and enable the link
  • Revisit all of the same non-existing urls

Expected result

update the hit count for all the non-existing urls

Actual result

updates the hit count only for the disabled links

@brianteeman
Copy link
Contributor

The code wasn't the issue. It's am easy fix. The question was if there was a reason

@richard67
Copy link
Member

I remember there was some discussion about that in past, but I don't remember when and where (issue or PR) ... but I think it might really be intended like it is. Maybe @wilsonge remembers? Or @zero-24 ?

@zero-24
Copy link
Contributor

zero-24 commented Jan 8, 2020

I can't help here.

@richard67
Copy link
Member

richard67 commented Jan 8, 2020

I have searched but haven't found it. Maybe I remember wrong, or maybe it was hidden in some issue or pr not directly related to the hit counter.

There are pros and cons.

A con is when counting redirect hits for a redirect which is enabled over years, the count might become very high. Maybe the count once was intended to show only how much a redirect would be needed, i.e. count the 404 only so I can see if it really needs to enable it or not.Tthis can be quickly seen with new collected urls by just ordering by the count, or descending by id and then verifying the counts. When having a counts for all redirects, maybe all with similar, high counts, it might be less easy to quickly see that.

A pro would be to see if a count did not increase anymore for a longer time for an enable redirect, then it could be removed maybe. But it would be easier to see that if it was possible to reset the counter so I can see after a while if it is still zero, then the redirect could be removed, instead of having to compare with an old counter value to see if it has changed or not.

I am not sure what is better, count only the 404 like now, or always count.

But especially if always count it would be good to have a way to reset the counter without having to use PhpMadmin or similar tools. Maybe it could always be reset when status changes from disabled to enabled or vice versa?

@brianteeman
Copy link
Contributor

A pro would be to see if a count did not increase anymore for a longer time for an enable redirect, then it could be removed maybe.

Thats why I was looking at this in the first place with a view to updating the modified date each time it was "hit" and thats when I discovered the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants