Skip to content

Improve handling of URLs and location redirects in AMP#1203

Merged
westonruter merged 7 commits intodevelopfrom
url-handling
Jun 7, 2018
Merged

Improve handling of URLs and location redirects in AMP#1203
westonruter merged 7 commits intodevelopfrom
url-handling

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter 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 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.

@westonruter
Add amp_get_current_url() helper function
5e0055a
@westonruter
Redirect from /amp/ URL endpoint to URL with ?amp query param in pair…
029a325 
…ed amp theme support
@westonruter
Let amp_add_amphtml_link() replace poorly-named amp_frontend_add_cano…
a7fbe8f 
…nical()

Deprecate amp-frontend-actions.php in favor of adding function to amp-helper-functions.php.
@westonruter
Fix old_slug_redirect_url handling for AMP theme support
31b9d52
@westonruter
Fix handling of re-validating URLs when redirects happen or post slug…
5ef42c2 
…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
Restore passing of current user cookies when validating a URL to acce…
16441b4 
…ss private/drafted posts

Partially reverts 90eff4f
@westonruter
Increase timeout for validate_url requests; show errors when fetching…
236be4b 
… fails

See #1166.
@westonruter westonruter added this to the v1.0 milestone Jun 7, 2018
@gravityrail
Copy link
Copy Markdown
Contributor

gravityrail commented Jun 7, 2018 •
edited
Loading

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
Copy Markdown
Member Author

westonruter commented Jun 7, 2018 •
edited
Loading

@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

😕

@westonruter westonruter mentioned this pull request Jun 7, 2018
Merged
@gravityrail
Copy link
Copy Markdown
Contributor

gravityrail commented Jun 7, 2018 •
edited
Loading

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
gravityrail approved these changes Jun 7, 2018
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Member Author

westonruter commented Sep 5, 2018

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

westonruter added a commit that referenced this pull request Oct 5, 2020
@westonruter
Remove obsolete /amp/=>?amp redirection in ensure_proper_amp_location()
cf221bd 
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

Reviewers

@amedina amedina Awaiting requested review from amedina

1 more reviewer

@gravityrail gravityrail gravityrail approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v1.0

Development

Successfully merging this pull request may close these issues.

3 participants

@westonruter @gravityrail @artkanna