Before anyone says "no new features" this really is a bug fix

Thanks @brianteeman I thought so too, but as I added a config option I felt I should declare it as a feature. I was surprised it didn't continue to track once published.

Shouldn't be an option, instead it should always count

If I can get confirmation people are happy for me to do it as default I’ll
remove the option and just always count.

It was my assumption it should always count, but at the same time I
appreciate it’s a function change to a point.

On Tue, 29 Jun 2021 at 21:24, Harald Leithner @.***>
wrote:

Shouldn't be an option, instead it should always count

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#34640 (comment),
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAKWBFRPJV4QCMKB24XZMVLTVITZLANCNFSM47NUS6HA
.

--
Tony Partridge
Xtech Web Services Ltd

Managing Director/Developer

Please see PR #27440.

Thanks Quy, seems like no one could agree on what to do 😫

Another option is we add a separate hits counter and column called Redirect
hits.

This would fall under new feature I feel. If I can get enough agreement
this would be accepted I’ll happily code it in.

Many thanks
Tony

On Wed, 30 Jun 2021 at 11:18, Quy @.***> wrote:

Please see PR #27440 #27440.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#34640 (comment),
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAKWBFXJ5TZ6LO3IGA3LOITTVLVPPANCNFSM47NUS6HA
.

--

Now updated to just always count, as Brian and HLeithber agreed this should also happen by default..

I have tested this item ✅ successfully on ed75f34

Looks good to me thanks!

I have tested this item ✅ successfully on ed75f34

Tested on Joomla 3.10.3-dev.

Thank you and best regards

RTC

Mhh, in general I agree with this PR, but when the saving failed, should we really kill the redirect just because the saving of the hit counter does not work?

updated as requested.

I literally just copied the original method to avoid issues.

If we want to adjust how Joomla! Does it natively then we can do that as a whole opposed to a bit her and a bit there on PRs?

--
Sent from Canary (https://canarymail.io)

On Tuesday, Nov 23, 2021 at 11:31 am, Phil E. Taylor @.*** @.***)> wrote:

@PhilETaylor commented on this pull request.

In plugins/system/redirect/redirect.php (#34640 (comment)):

@@ -285,6 +285,17 @@ private static function doErrorHandling($error) // In case the url contains double // lets remove it $destination = str_replace(JUri::root() . '/', JUri::root(), $dest); + // Always count redirect hits + $redirect->hits++;
⬇️ Suggested change

• $redirect->hits++; + ++$redirect->hits;

Im not sure about others, but incrementing on a line by its own, I have always used the pre and not post version of ++ (Yes I understand the difference, and yes, its probably ok this way

Looking at more professional projects and they use ++$value

Joomla has both versions, and its not documented in the coding standards, except by accident in the A do-while Example

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub (#34640 (review)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAKWBFUS5B7ALTZKT7AEPILUNN3SFANCNFSM47NUS6HA).

If we want to adjust how Joomla! Does it natively then we can do that as a whole opposed to a bit her and a bit there on PRs?

Joomla has repeatedly rejected such PR's that seek to make code style changes throughout the code base :) Like I said before deleting, its not an issue, and as this is already RTC it can stay. Although:

This branch is out-of-date with the base branch

^^ that needs addressing before it can be merged.

Thanks, Phil. I have now merged changes.

Merging here thanks :)