Change ?amp to ?amp=1 #1390
Change ?amp to ?amp=1 #1390
Conversation
| @@ -85,7 +85,7 @@ function amp_get_permalink( $post_id ) { | |||
| if ( current_theme_supports( 'amp' ) ) { | |||
| $permalink = get_permalink( $post_id ); | |||
| if ( ! amp_is_canonical() ) { | |||
| $permalink = add_query_arg( 'amp', '', $permalink ); | |||
| $permalink = add_query_arg( 'amp', '1', $permalink ); | |||
There was a problem hiding this comment.
If we go ahead with this PR, then what we should do is take the opportunity to factor out the redundant formation of the AMP URLs with a new common function. There is presently amp_get_permalink() but that is specifically for getting URLs for posts. The amp_get_url() function would be used inside of amp_get_permalink().
There could be an amp_get_url() function that looks like this:
/**
* Get the AMP version of a URL.
*
* This simply appends the required query parameter to the URL. It does not confirm
* whether or not AMP is allowed on the given URL. If AMP is not supported, then a redirect
* to the non-AMP version will ensue.
*
* @see amp_get_permalink()
* @return string AMP URL.
*/
function amp_get_url( $url ) {
return add_query_arg( amp_get_slug(), '1', $url );
}There was a problem hiding this comment.
@westonruter I'll work on it...
There was a problem hiding this comment.
@ricardobrg are you working on this?
There was a problem hiding this comment.
Hello @westonruter! Getting back to this now...
|
@ricardobrg Would you still be interested in bringing this into the finish line? 🙂 |
|
@swissspidy yes! It is in my "todo list". I need to update the code and work in the feature. Give me one more week and I'll have another commit to this PR. |
|
I think we'll need to step back and re-consider this at a higher level. There are some negative side effects to changing the AMP URLs. See #2204. I suggest we close this PR and take into account all the points in that issue in a new PR. It will require deeper changes than just changing the URLs. |
|
Since this PR is almost 1 year old and lots of changes were made in the plugin, it will need a deeper work anyway. I will follow @westonruter suggestion and start working from #2204 considering the other related issues. |
Usually the static cache configuration (nginx, static html files, proxy, etc) tells the server to bypass cache if the query string is not empty. Since 1.0 the amp URLs use ?amp and that prevents the server from caching it.
It's really easy to add a rule that tells the server to cache a page if a non-empty query arg is present but it's not easy to work with empty query args.
I've changed
ampquery var toamp=1to make it possible to the server or cache and CDN services detect if amp query arg is present and deliver cached content.Fixes #1383.