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

Projects
None yet
3 participants
@westonruter
Copy link
Member

westonruter 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(). See #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.

westonruter added some commits Jun 6, 2018

Let amp_add_amphtml_link() replace poorly-named amp_frontend_add_cano…
…nical()

Deprecate amp-frontend-actions.php in favor of adding function to amp-helper-functions.php.
Fix handling of re-validating URLs when redirects happen or post slug…
…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.
Restore passing of current user cookies when validating a URL to acce…
…ss private/drafted posts

Partially reverts 90eff4f

@westonruter westonruter requested review from gravityrail and amedina Jun 7, 2018

@westonruter westonruter added this to the v1.0 milestone Jun 7, 2018

@gravityrail

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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)

@gravityrail
Copy link
Contributor

gravityrail left a comment

This works :)

@westonruter westonruter merged commit 6e6275b into develop Jun 7, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the url-handling branch Jun 7, 2018

@artkanna

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

westonruter commented Sep 5, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.