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

Improve handling of URLs and location redirects in AMP #1203

Merged
merged 7 commits into from
Jun 7, 2018

Conversation

westonruter
Copy link
Member

  • Automatically redirect from /amp/ endpoint to ?amp when amp theme support is present. This is important for plugins to be able to reliably call is_amp_endpoint(). See Discontinue use of amp endpoint in favor of query var when amp theme support is present #1148.
  • Add amp_get_current_url() helper function.
  • Let amp_add_amphtml_link() replace the poorly-named amp_frontend_add_canonical() function and improve how it is referenced.
  • Fix old_slug_redirect_url handling for AMP theme support. This ensures that AMP URLs redirect properly when changing a post's slug.
  • Fix handling of re-validating URLs when redirects happen or post slugs change. Fix validation when switching between native and paired modes. Only store canonical URLs for amp_invalid_url posts.
  • Restore passing of current user cookies when validating a URL to access private/drafted posts.
  • Increase timeout for validate_url requests; show errors when fetching fails
  • Fix issue with get_current_screen() not returning an object.

Fixes #1166.

…nical()

Deprecate amp-frontend-actions.php in favor of adding function to amp-helper-functions.php.
…s change

* Fix validation when switching between native and paired modes.
* Only store canonical URLs for amp_invalid_url posts.
* Fix issue with get_current_screen() not returning an object.
@westonruter westonruter added this to the v1.0 milestone Jun 7, 2018
@gravityrail
Copy link
Contributor

gravityrail commented Jun 7, 2018

Automatically redirect from /amp/ endpoint to ?amp when amp theme support is present. This is important for plugins to be able to reliably call is_amp_endpoint().

I'm not able to reproduce this behaviour. I tried with/without add_theme_support( 'amp' ), with/without trailing /amp/, and I wasn't able to trigger a redirect to ?amp. I see that you have hooked your link redirection code to the filter old_slug_redirect_url but I don't see anywhere in your branch that that filter is called - maybe there's a change missing from this PR?

@westonruter
Copy link
Member Author

westonruter commented Jun 7, 2018

@gravityrail humm, the old_slug_redirect_url is for a different case for when the slug changes. The redirection from /amp/ to ?amp is performed here in the ensure_proper_amp_location method:

https://github.com/Automattic/amp-wp/blob/236be4babd249e27b98a60526355d036ef4872d4/includes/class-amp-theme-support.php#L166-L206

I just double-checked and it's working for me:

$ curl -i https://src.wordpress-develop.test/2018/06/06/initial333/amp/
HTTP/1.1 302 Found
Location: https://src.wordpress-develop.test/2018/06/06/initial333/?amp

😕

@gravityrail
Copy link
Contributor

gravityrail commented Jun 7, 2018

Ah! Now I'm able to reproduce the desired behaviour.

The trick is to enable paired mode, which requires specifying a theme directory to AMP-ify:

add_theme_support( 'amp', array( 'template_dir' => './' ) );

Then you ALSO need to make sure you accept any validation errors that have been generated (otherwise it always redirects from AMP to non-AMP versions of page)

Copy link
Contributor

@gravityrail gravityrail left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works :)

@westonruter westonruter merged commit 6e6275b into develop Jun 7, 2018
@westonruter westonruter deleted the url-handling branch June 7, 2018 17:09
@artkanna
Copy link

artkanna commented Sep 5, 2018

Hi! Will Fix old_slug_redirect_url handling for AMP theme support retroactively fix amp redirects for pages that were previously redirected - before the release? Thank you!

@westonruter
Copy link
Member Author

@artkanna Can you give an example of pages that were previously redirected?

westonruter added a commit that referenced this pull request Oct 5, 2020
This logic was originally added in #1203 for #1148, but that was ultimately reverted in #1235.
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.

3 participants