statify
statify copied to clipboard
pluginkollektiv
Wrong path for AMP pages
Currently, for AMP pages the URL gets extended by an amp/ (https://github.com/pluginkollektiv/statify/blob/develop/inc/class-statify-frontend.php#L428). However, this will result in wrong URLs since https://example.com/ will become https://example.com/amp/ in the database. The correct way would be to add an ?amp or & to the URL to get a valid URL, which is the AMP variant of the page.
Both ?amp and /amp are potentially AMP requests, but you're right, not in this scenario. /amp/ with trailing slash is likely to be always wrong.
We already do assume a permalink structure without arguments when we use ${canonicalPath}amp/. So I'd suggest do deal with it and simply change it to ${canonicalPath}?amp. We can extend the check to potentially use & if necessary. Any different thoughts on that?
In terms of "which of my posts is accessed how often" the explicit AMP parameter is not optimal anyway and subject to be changed in the context of #104 (mark mobile usage) where AMP calls could be marked differently.
Apparently we do some cleanup that removes a trailing ?amp from the target variable:
https://github.com/pluginkollektiv/statify/blob/23f9e3648957b17bb91cba77419e043ed87098bd/inc/class-statify-frontend.php#L94-L100
Original:
/root/my-page/?amp
After "generation of relative target URL" including user_trailingshashit():
/my-page/?amp/
Then, if permalink structure is used, we trim arguments:
/my-page/
So the information is lost while trailing /amp is not :thinking:
We would have to parse the query here, ~match to something like ^([^&]+&)*amp(&[^&]+)*$ to detect a valid AMP parameter~ check if it equals amp/ (${canonicalPath} already strips other parameters) and re-add it to the target value.
However /amp or /amp/ is also potentially valid to retrieve an AMP page. So we don't necessarily do anything "wrong" here.
On my test site (Official plugin 2.0, Standard mode, no special config) all three calls redirect to the actual page served through AMP. Apparently that's not always correct as your site shows a 404 instead.
Is this AMP issue fixed with #182 released with Statify 1.8.1 today?
The reference commit fa211b4 should solve the issue. But as stated before, I’m not entirely happy with the solution, because all 3 variants are potentially valid. No feedback so far.
Another solution might be dropping the AMP suffix completely. The displayed content is more or less the same and we do not interpret mobile/desktop views either, no matter if they differ, so why should we treat AMP in a special way? (or if, why not considering all, see #104)
Imo the decision is not really what is “correct“, but more what we want to express here and how.
At least the variant with & doesn’t work on my page. E. g. https://kittmedia.com/&
It results in a 404.
But yes, if you don’t want to have any difference (with what I could easily live with), omitting these value from the URL entirely would be an easy option. As long as it’s still counted as page view. 😄
@stklcode Your commit fa211b4 may produce a notice:
ErrorException: Notice: Undefined index: query
#5 /wp-content/plugins/statify/inc/class-statify-frontend.php(107): Statify_Frontend::track_visit
#4 /wp-content/plugins/statify/inc/class-statify-frontend.php(148): Statify_Frontend::track_visit_ajax
#3 /wp-includes/class-wp-hook.php(287): WP_Hook::apply_filters
#2 /wp-includes/class-wp-hook.php(311): WP_Hook::do_action
#1 /wp-includes/plugin.php(484): do_action
#0 /wp-admin/admin-ajax.php(199): null
It seems you also need to check for isset( $parsed_target['query'] ) in line 106.
I think that's what I actually wanted to check in this line :see_no_evil: $parsed_target is accessed a couple of lines before and probably never unset.