-
Notifications
You must be signed in to change notification settings - Fork 384
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
Add customization of Paired AMP URLs #5558
Conversation
c5300ef
to
12378ae
Compare
src/PairedAmpRouting.php
Outdated
$has_filters = [ | ||
has_filter( 'amp_has_paired_endpoint' ), | ||
has_filter( 'amp_add_paired_endpoint' ), | ||
has_filter( 'amp_remove_paired_endpoint' ), | ||
]; | ||
$has_filter_count = count( array_filter( $has_filters ) ); | ||
if ( 3 === $has_filter_count ) { | ||
return self::PERMALINK_STRUCTURE_CUSTOM; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted by @schlessera, we may want to switch this around so that the filters do not force the custom URL structure, but rather that the filters can merely allow the custom structure to be among the selectable options.
d9b724b
to
e30675f
Compare
I've moved the Custom structure to a notice and added example URLs for each structure: When Custom URLs are not being used: I'm not totally happy with how these options are laid out, but the information is there at least. /cc @jwold |
ae9fc89
to
3f9bc7f
Compare
Making ready for review to create build ZIPs. |
6b5f2d5
to
6c648b0
Compare
Plugin builds for 180fdbc are ready 🛎️!
|
Maybe we could configure a slash command that triggers the build jobs on demand. |
76e86b0
to
a418cc9
Compare
@@ -151,7 +156,7 @@ public function test_redirect_on_transitional_and_not_available() { | |||
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG ); | |||
AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false ); | |||
AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ 'is_author' ] ); | |||
$this->go_to( amp_add_paired_endpoint( '/' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to the AMP endpoint seems to have been a bug here. I'm unclear why it was passing tests before.
@@ -185,7 +190,8 @@ public function test_redirect_when_server_side_and_not_applicable() { | |||
add_filter( 'amp_mobile_client_side_redirection', '__return_false' ); | |||
add_filter( 'amp_pre_is_mobile', '__return_false' ); | |||
|
|||
$this->go_to( amp_add_paired_endpoint( '/' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to the AMP endpoint seems to have been a bug here. I'm unclear why it was passing tests before.
e9348a1
to
52a30cc
Compare
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
tests/php/src/PairedUrlStructure/LegacyTransitionalUrlStructureTest.php
Outdated
Show resolved
Hide resolved
tests/php/src/PairedUrlStructure/LegacyReaderUrlStructureTest.php
Outdated
Show resolved
Hide resolved
tests/php/src/PairedUrlStructure/PathSuffixUrlStructureTest.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
…rl-customization * 'develop' of github.com:ampproject/amp-wp: Use wp_robots() instead of noindex() in WP 5.7 (#5793) Bump eslint-plugin-jsdoc from 31.0.3 to 31.0.4 Bump google/cloud-storage from 1.23.0 to 1.23.1 Bump @wordpress/block-editor from 5.2.0 to 5.2.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A thing of beauty, well done!
@pierlon Thank you for the thorough review and for getting the tests across the finish line. 🤝 |
Follow up PR to fix an issue where a URL with an empty query param (e.g. |
Fixes #2204
Fixes #2062
Fixes #3357
This implements the customizability of a site's paired AMP URL structure, one of our most-frequently requested capabilities and one which will facilitate sites migrating from other AMP plugins.
Paired URL Structures
A new admin settings section is introduced for "Paired URL Structure" which allows the user to select from among four built-in paired URL structures:
?amp=1
is used on all URLs. This is the default for new installs and it is the recommended structure since it does not conflict with any rewrite rules./amp/
is used on all URLs. This is available due to popular demand (e.g. vanity reasons), although it offers no functional benefits over?amp
.?amp
is used on all URLs. When upgrading the plugin from 2.0.x, a site in Transitional mode (or Reader mode with a Reader theme) will default to this structure./amp/
is used on all non-hierarchical posts and?amp
is used on everything else. When upgrading the plugin from 2.0.x, a site in Transitional mode will default to this structure. (Note: This is the only structure that continues to support theamp_pre_get_permalink
andamp_get_permalink
filters.)Each structure includes examples of how they will look:
In the case of the path suffix and legacy reader paired URL structures (which both utilize
/amp/
path segment URL suffix), these structures will be unavailable if there is any entity on the site is currently using theamp
slug (whether a post, term, user, post type, etc):When either the path suffix and legacy reader paired URL structures are active, then a
wp_unique_post_slug
filter is used to preventamp
from being used a post slug, forcing it rather to be disambiguated likeamp-2
. (Again, the potential for conflicts is why the query parameter paired URL structure is recommended.)In addition, users can implement their own paired URL strategy by extending the
AmpProject\AmpWP\PairedUrlStructure
class and supplyingthis class name via the
amp_custom_paired_url_structure
filter. The subclass must implement two methods:add_endpoint
andremove_endpoint
.A
has_endpoint
method can also be implemented, but the base class has a default implementation that usesremove_endpoint
. For examples of how custom paired URLstructures can be implemented, see these mini plugins:
https://amp.example.com/foo/
)https://example.com/amp/foo/
)In order for the custom URL structure to work, the
amp_custom_paired_url_structure
filter must be added before theplugins_loaded
action at priority 7 (since the paired structure has to be set at this point to be used byPairedRouting
). So the best bet is to add the filter at the top-level of your plugin and not inside of an action. For example:The same goes for filtering
amp_query_var
.When a custom paired URL structure is used, all of the built-in structures are disabled and a notice is displayed:
Paired Routing
Previously
?amp=1
paired URLs were the exclusive structure available when using a Reader theme because theReaderThemeLoader
service needed to rely on$_GET['amp']
as a signal for whether to switch the theme in the request. It was not able to use the/amp/
“rewrite endpoint” in WordPress because that depends on theWP::parse_request
having been run, which is not available early enough (atplugins_loaded
) as a signal forReaderThemeLoader
. In order to allow for any paired URL structure to be used in any template mode (either Transitional, Reader theme, or legacy Reader), WordPress's rewrite rules are not used at all for detecting paired AMP URLs. Instead, the newPairedRouting
service will examine the URL for matching against the currently-selected paired URL structure and store whether it is a paired AMP request. The paired AMP endpoint is then removed from the environment (e.g.$_SERVER['REQUEST_URI']
) whileWP::parse_request()
runs so that all current rewrite rules will work as they do normally for non-AMP requests. Theamp
query var is added to theWP::$query_vars
. Then at once theWP::parse_request()
process is finished at theparse_request
action, then the endpoint is re-added to$_SERVER['REQUEST_URI']
as well as$wp->request
.To reiterate: Previously this plugin implemented the
/amp/
URL endpoint suffix by means of callingadd_rewrite_endpoint()
in the WordPress rewrite API. This worked adequately for singular post URLs, but it quickly became clear that it would not be suitable for use across all URLs on a given site. Even whenEP_ALL
is supplied toadd_rewrite_endpoint()
the reality is that all rewrite rules do not get the endpoint: for example paged URLs , comment page URLs, and URLs which contain other endpoints. For these reasons, the use of rewrite rules to implement paired URL structures was eliminated entirely in favor of inspecting theREQUEST_URI
for the path segment or query parameter (or some other environment indicator). When WordPress finally gets to runningWP::parse_request()
, the AMP endpoint is stripped from the environment (i.e.$_SERVER['REQUEST_URI']
) just before at thedo_parse_request
hook so that all of the site's rewrite rules will continue to match as if AMP was not being requested at all. Then after the rewrite rules have been matched at theparse_request
action, the AMP endpoint is restored in the environment. The ability to identify paired AMP URLs before thewp
action allows for all paired URL structures to be used even when using a Reader theme (which needs to know if the request is for an AMP URL atplugins_loaded
in order to switch the theme). In addition to this allowing AMP endpoints to work with all URLs matched by a site's rewrite rules, it also means there is no longer a need to flush rewrite rules when activating (or deactivating) the AMP plugin.The work on paired routing resulted paying down a good amount of tech debt namely be moving a lot of code from functions and the massive
AMP_Theme_Support
class into their respective.Paired Browsing
When Paired Browsing was first implemented (#3656), the only paired URL structure applicable was the legacy Transitional mode (
?amp
). This meant it was trivial for the Paired Browsing JS code to obtain and identify an AMP URL from a non-AMP one and vice versa, since it meant just adding or removing theamp
query parameter. With the introduction of customizable paired URL structures, this complicated matters, the biggest being that a paired URL structure may involve a separate domain entirely (e.g. anamp
subdomain). The fact that the AMP half of the Paired Browsing UI could be in an iframe that's on another domain meant that we could no longer rely on direct cross-window access in JS to synchronize the current URL and the scroll position: it was necessary to make use thepostMessage
API instead, in the same way that the Customizer does. So this PR includes this major refactor of Paired Browsing.The refactor included extraction of a lot of code from the massive
AMP_Theme_Support
class into the newPairedBrowsing
service.Post Previews
Similarly to the changes necessitated for Paired Browsing, the ability to view the AMP version of a Post Preview also needed to be changed. No longer could the AMP version of a post be obtained just by adding the
amp
query parameter. So now the AMP version of the post preview URL is obtained server-side and exported to the client for use in the Preview button.Additionally, the editor plugin that introduces the AMP preview button is now skipped from registration when the site is in Standard mode. Query parameters which would be added to the preview URL (e.g.
preview_nonce
andpreview_id
) are copied on top of that initial AMP preview URL.Changes for
canonical
linksPreviously when a non-AMP page lacked a
link[rel=canonical]
element, it would attempt to generate an actual “canonical” URL to supply for the requiredlink
added to AMP pages. In reality, in an AMP context this “canonical” link does not mean semantic canonicity but rather simply that it is “non-AMP”. Therefore, the logic has been changed so that on AMP pages, thecanonical
link is simply the current URL with the AMP endpoint removed when in a paired AMP mode (Transitional or Reader) and to just return the current URL when in Standard mode. In order to output actual semanticallycanonical
links, add a plugin like Yoast SEO or advocate for core to output canonical links on non-singular responses (see TRAC-18660).Mobile Redirection
If a site had mobile redirection enabled in Reader mode or Transitional mode and then they switched to Standard mode, the mobile redirect option would remain enabled. The
MobileRedirection
service was not checking whether Standard moe was active, resulting in needless hooks added for when a paired mode is active. This has been rectified in 2474e8d.This PR also ensures that submitting a comment an AMP page will cause the redirected URL to be an AMP page, as opposed to the non-AMP page.
404 Handling
When accessing a URL results in a 404 and and the URL also ends in the
/amp/
URL suffix, the plugin will automatically try redirecting to/?amp=1
. This ensures that sites that formerly used the/amp/
URL suffix will not end up with broken URLs when switching. Fixes #2062.Old Slug Redirect Handling
When the 404 template was not enabled for AMP, attempting to access the AMP version of a URL which had the old post slug would end up getting redirected to the new permalink but without the AMP endpoint. This was due to the
redirect_paired_amp_unavailable
running first to stip off the AMP endpoint when it detected the 404, beforewp_old_slug_redirect()
was able to run to redirect with the AMP endpoint persisted. So now inredirect_paired_amp_unavailable
just before redirection happens we just-in-time callwp_old_slug_redirect()
to make sure its redirection takes precedence. Fixes #3357.New APIs
New Classes
\AmpProject\AmpWP\PairedRouting
: Service for routing users to and from paired AMP URLs.\AmpProject\AmpWP\Admin\PairedBrowsing
: Service for the Paired Browsing app.\AmpProject\AmpWP\PairedUrl
: Service for common manipulations for paired URLs, such as whether a given URL has a query var or endpoint suffix, or to remove/add them.\AmpProject\AmpWP\PairedUrlStructure
: Abstract class which which a paired URL extends to describe how to make a URL paired, make it un-paired, and to determine if it is paired.\AmpProject\AmpWP\LegacyReaderUrlStructure
: Legacy Reader paired URL structure.\AmpProject\AmpWP\LegacyTransitionalUrlStructure
: Legacy Transitional paired URL structure.\AmpProject\AmpWP\QueryVarUrlStructure
: Query Var paired URL structure.\AmpProject\AmpWP\PathSuffixUrlStructure
: Path Suffix paired URL structure.Hook Changes
amp_default_options
filter now passes the current$options
as the second parameter.amp_validated_url_status_actions
filter allows services to inject links into the publish metabox on the validated URL screen.amp_rest_options
filter is introduced for services to add (readonly) options to the AMP settings response.amp_rest_options_schema
filter is introduced for services to describe their own options.Deprecated Functions
amp_correct_query_when_is_front_page()
, now handled byPairedRouting
.amp_add_frontend_actions()
, now handled byPairedRouting
.amp_redirect_old_slug_to_new_url()
, now handled byPairedRouting
.Plugin Suppression
The Plugin Suppression logic now defers initialization until
plugins_loaded
priority 9 as opposed to running much earlier at the minimum priority. This is because it needs to check ifhas_endpoint
which depends on plugins possibly registering their own custom paired URL structure, say atplugins_loaded
priority 0. (2d4dea4)/amp/
addition is added properly./amp/
.https://wordpressdev.lndo.site/blog/amp/
to next page athttps://wordpressdev.lndo.site/blog/amp/page/2/?amp=1
.Consider removing legacy options.(To do in future.)/amp/
invoke canonical redirect or should it strip the endpoint?PairedAmpRouting
./amp/
and?amp
.?amp=1
from URLs when AMP is canonical./amp/
to$wp->request
.PairedAmpRouting
into more services.amp_custom_paired_url_structure
filter is added too late?Checklist