Default to ?amp=1 query parameter #4312

pierlon · 2020-02-18T13:03:19Z

Summary

Closes #4442
Relates to #2204

This PR enforces the ?amp=1 query parameter throughout the plugin.

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

includes/class-amp-theme-support.php

includes/amp-helper-functions.php

westonruter · 2020-02-24T06:37:39Z

@schlessera in regards to:

I usually prefer the PHP filtering functions for dealing with $_GET and $_POST. I think this woud also remove the need for the PHPCS ignore rule.

How do you unit test these? AFAIK you can't manipulate the underlying data source that filter_input() reads from.

…mp_location

pierlon · 2020-08-30T05:14:18Z

Starting this PR anew; this will focus on enforcing the ?amp=1 query parameter, and will then be followed by a separate PR focusing on the customization of AMP URLs.

github-actions · 2020-08-30T05:30:04Z

Plugin builds for ac94637 are ready 🛎️!

Download development build
Download production build

includes/amp-helper-functions.php

westonruter · 2020-09-05T05:34:22Z

includes/amp-helper-functions.php

+ * @return bool True if the AMP query parameter is set with the required value, false if not.
+ */
+function amp_has_query_var() {
+	return isset( $_GET[ amp_get_slug() ] ) && 1 === filter_var( $_GET[ amp_get_slug() ], FILTER_VALIDATE_INT ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended


I don't think the value should need to be required to be 1. It is just 1 as a generic non-empty value.

Suggested change

return isset( $_GET[ amp_get_slug() ] ) && 1 === filter_var( $_GET[ amp_get_slug() ], FILTER_VALIDATE_INT ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended

return isset( $_GET[ amp_get_slug() ] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended

includes/amp-helper-functions.php

westonruter · 2020-09-05T05:39:20Z

includes/class-amp-theme-support.php

-			if ( isset( $path_args[ amp_get_slug() ] ) && '' !== $path_args[ amp_get_slug() ] ) {
+			if ( isset( $path_args[ amp_get_slug() ] ) && '1' !== $path_args[ amp_get_slug() ] ) {


I don't think this change is quite right. This code here is asking if the URL looks like this /foo/amp/bar/, where there is something after the /amp/ (like for feeds there is /feed/rss/ and /feed/atom/). The $path_args is not the query vars from $_GET but rather wat was matched from the rewrite rule. So for a request /foo/amp/bar/, here $path_args[ amp_get_slug() ] would be set to bar. If it is any non-empty string value, then this indicates a redirect is needed to avoid serving an infinite URL space.

Yes that makes sense. I've made the changes in 83ec4a8.

includes/class-amp-theme-support.php

Co-authored-by: Weston Ruter <westonruter@google.com>

…nt/2204-default-amp-endpoint * 'develop' of github.com:ampproject/amp-wp: (386 commits) Improve indentation Only show warning notice to users with `manage_options` capability Reword notice Include note as to why the WP and GB versions were chosen Sanitize translation Let the user know if the current Gutenberg or WordPress version is not supported Update dependency @wordpress/eslint-plugin to v7.2.1 Update dependency eslint-plugin-jsdoc to v30.6.3 Update dependency copy-webpack-plugin to v6.2.0 Update dependency @babel/plugin-proposal-class-properties to v7.10.4 Update dependency eslint-plugin-import to v2.22.1 Add missing docblocks Fix image importing for travel blog Update dependency mini-css-extract-plugin to v0.11.3 Update dependency postcss-loader to v4.0.3 Fix style difference between Gutenberg and Core Revert "Fix visual inconsistency between Core and Gutenberg" Fix visual inconsistency between Core and Gutenberg Make download errors visible Adapt image links in travel blog site ...

This logic was originally added in #1203 for #1148, but that was ultimately reverted in #1235.

westonruter · 2020-10-05T23:40:04Z

includes/amp-helper-functions.php

+ * @todo Rename this to clear up confusion with amp_get_current_url().
+ *
+ * @param string $url URL.
+ * @return string AMP URL.
+ */
+function amp_get_url( $url ) {


We have amp_get_current_url() and amp_get_asset_url() and amp_get_customizer_url(). These being here, I think amp_get_url() is too ambiguous.

How about we call it amp_get_paired_url() since its purpose is to get the paired AMP URL for the current page.

We also have a \AMP_Theme_Support::get_current_canonical_url() which could be extracted into a separate global function called amp_get_canonical_url().

Done in b66c233

westonruter · 2020-10-06T01:18:31Z

includes/amp-helper-functions.php

+ * @todo Rename this to be more agnostic to what the URL contains.
+ *
+ * @return bool True if the AMP query parameter is set with the required value, false if not.
+ * @global WP_Query $wp_query
+ */
+function amp_has_query_var() {


Given that in the future a paired AMP URL will be able to be indicated arbitrarily, such as via a subdomain, I think we can anticipate that by making amp_has_query_var() more abstract.

We have amp_is_request() to indicate that the request is being made for an AMP page. What we need is a method that says that the request was made for a paired AMP URL. What about amp_is_paired_request()? I'm not entirely satisfied with that since it implies that it entails amp_is_request(), being more specific. It's actually just giving an indication that the current URL is for paired AMP.

Maybe amp_is_paired_url()? We could pass it a $url argument as well, and it could tell you if that URL is paired rather than the current request to WP. This would be useful in other contexts, like in the AMP-to-AMP Linking sanitizer perhaps.

Thinking about this some more… what about amp_is_paired_endpoint? Using “endpoint” terminology would align with what we already have in amp_remove_endpoint() (which we could technically rename to amp_remove_paired_endpoint()).

While in a WordPress context, an “endpoint” refers to the end of the URL, in reality an API endpoint doesn't necessarily refer to any physical part of the URL. Normally the endpoint will still be at the end (?amp=1) but sites can customize the paired AMP endpoint URLs to look however they want.

I suppose then we'd rename amp_get_paired_url( $url ) to amp_get_paired_endpoint( $url ).

We basically need CRUD functions for manipulating paired AMP URLs:

Read: has/is

Create/Update: get

Delete: remove

So:

Read: amp_is_paired_endpoint( $url )

Create/Update: amp_get_paired_endpoint( $url )

Delete: amp_remove_paired_endpoint( $url ) (formerly amp_remove_endpoint())

Thoughts?

Using “endpoint” terminology would align with what we already have in amp_remove_endpoint() (which we could technically rename to amp_remove_paired_endpoint()).

That reasoning makes sense. I'm also in support of renaming the functions to represent the CRUD methods of manipulating the paired URL 👍.

Sidenote: That means amp_remove_endpoint() would have to be deprecated and annotated accordingly since its documented.

Here, check this out: d5610f2

…nt()

westonruter

Fixes #2062

It appears that this does not actually fix that issue.

With Legacy AMP Reader mode active, I tried going to /category/uncategorized/amp/ and I was taken to a 404 not being redirected to /category/uncategorized/?amp=1 as I expected.

@pierlon That being the case, should we just remove that Fixes to address in another PR?

westonruter · 2020-10-07T04:00:00Z

tests/php/test-amp-helper-functions.php

-			$this->assertStringEndsWith( '&amp', amp_get_permalink( $published_post ) );
-			$this->assertStringEndsWith( '&amp', amp_get_permalink( $drafted_post ) );
-			$this->assertStringEndsWith( '&amp', amp_get_permalink( $published_page ) );
+
+			remove_filter( 'amp_pre_get_permalink', [ $this, 'return_example_url' ] );
+			remove_filter( 'amp_get_permalink', [ $this, 'return_example_url' ] );
+			$this->assertStringEndsWith( '&amp=1', amp_get_permalink( $published_post ) );
+			$this->assertStringEndsWith( '&amp=1', amp_get_permalink( $drafted_post ) );
+			$this->assertStringEndsWith( '&amp=1', amp_get_permalink( $published_page ) );
 			add_filter( 'amp_get_permalink', [ $this, 'return_example_url' ], 10, 2 );
-			$this->assertStringNotContains( 'current_filter=amp_get_permalink', amp_get_permalink( $published_post ) ); // Filter does not apply.
+			$this->assertStringEndsWith( 'current_filter=amp_get_permalink', amp_get_permalink( $published_post ) );
 			add_filter( 'amp_pre_get_permalink', [ $this, 'return_example_url' ], 10, 2 );
-			$this->assertStringNotContains( 'current_filter=amp_pre_get_permalink', amp_get_permalink( $published_post ) ); // Filter does not apply.
+			$this->assertStringEndsWith( 'current_filter=amp_pre_get_permalink', amp_get_permalink( $published_post ) );


Nice. Looks like you found a bug in the test here.

pierlon · 2020-10-07T05:02:04Z

@pierlon That being the case, should we just remove that Fixes to address in another PR?

Yes let's do that. I'll update the PR description.

Also, reviewed the recent changes and it's a 👍 from me.

schlessera · 2020-10-08T17:36:37Z

I find the naming to be confusing, for the reason that get usually represents the READ of CRUD, not the CREATE/UPDATE.

I don't think it's a bad idea to use is, get & remove, necessarily, but it confuses when referred to as CRUD operations.

westonruter · 2020-10-08T17:38:23Z

I don't think it's a bad idea to use is, get & remove, necessarily, but it confuses when referred to as CRUD operations.

OK, that makes sense. In any case, there is no reference to CRUD in the codebase.

schlessera · 2020-10-08T17:40:28Z

Alright. CRUD really is about manipulating entities. That is not the case here.

That being said, I think the names are good and make sense to me.

schlessera · 2020-10-08T17:43:00Z

I wonder whether it is clear for users, though...

I was just now thinking about the following constellation:

amp_is_paired_url()
amp_get_paired_url()
amp_get_source_url()

westonruter · 2020-10-08T17:49:51Z

That would work, though amp_get_source_url() I'm not sure about, in regards to the “source”. In particular, it would make more sense to use amp_get_canonical_url() since the URL returned would correspond with the link[rel=canonical] on the AMP page. (But then this may not actually correspond with the link[rel=canonical] on the canonical page.)

schlessera · 2020-10-09T07:07:05Z

Yes, the following would work:

amp_is_paired_url()
amp_get_paired_url()
amp_get_canonical_url()

I guess I have an issue with the remove_endpoint bit, as it sounds like disabling something or deleting something. By having two get_*() variations, it makes it clear that you're getting a URL in both cases, it's just that we happen to have two variations of that URL.

westonruter · 2020-10-09T07:10:55Z

My only concern with amp_get_canonical_url() is users may expect that its return value will be the same as what is returned by wp_get_canonical_url() but this won't necessarily be the case. Really it's about getting the “unpaired” URL.

westonruter · 2020-10-09T19:55:20Z

Here's an alternative suggestion:

amp_has_paired_endpoint( $url ) (instead of is)
amp_add_paired_endpoint( $url ) (instead of get)
amp_remove_paired_endpoint( $url )

westonruter · 2020-10-09T20:44:17Z

Note we currently have amp_remove_endpoint().

westonruter · 2020-10-09T23:04:17Z

cf. add_query_arg() and remove_querg_arg() in core.

…nt/2204-default-amp-endpoint * 'develop' of github.com:ampproject/amp-wp: Fix hard-coded SSR tests Change string rendition of float numbers for sizers Update spec tests Ensure iframes in embeds with aspect ratios get responsive layout (#5486) Include additional sandbox tokens for converted iframes (#5483) Fix alignment for phpcs Update dependency sirbrillig/phpcs-variable-analysis to v2.9.0 Fix PHPStan issue Update dependency terser-webpack-plugin to v4.2.3 Update dependency mini-css-extract-plugin to v0.12.0 Add issue reference to TODO Add expiry to stylesheet cache transients that exceed cache expiry Add test to assert stylesheet cache is not autoloaded Fix rendering translations in JS (#5461) Extract regex into constant Add tests for other doctypes Fix PSR2.Methods.FunctionCallSignature Ensure at least one space after doctype Always normalize to use HTML5 doctype

westonruter · 2020-10-10T20:36:34Z

I'm happy with the naming but we can adjust further in a subsequent PR.

googlebot added the cla: yes Signed the Google CLA label Feb 18, 2020

pierlon requested review from westonruter, schlessera and kienstra February 18, 2020 13:03

schlessera requested changes Feb 21, 2020

View reviewed changes

schlessera added the WS:Core Work stream for Plugin core label Aug 18, 2020

pierlon added 2 commits August 29, 2020 18:38

Remove unused parameter in call to AMP_Theme_Support::ensure_proper_a…

4b770da

…mp_location

Default to ?amp=1 query parameter

d0ea373

pierlon force-pushed the enhancement/2204-default-amp-endpoint branch from 9455eaa to d0ea373 Compare August 30, 2020 05:14

Remove amp_url filter

f806254

pierlon changed the title ~~Add option to override AMP URL~~ Default to ?amp=1 query parameter Aug 30, 2020

pierlon removed the request for review from kienstra August 30, 2020 05:26

pierlon marked this pull request as ready for review August 30, 2020 05:26

pierlon requested a review from schlessera August 30, 2020 05:27

pierlon added this to the v2.1 milestone Aug 30, 2020

pierlon self-assigned this Aug 30, 2020

westonruter requested changes Sep 5, 2020

View reviewed changes

westonruter reviewed Sep 5, 2020

View reviewed changes

includes/class-amp-theme-support.php Outdated Show resolved Hide resolved

pierlon and others added 7 commits September 21, 2020 20:22

Revert making the value '1' required for the AMP query var

83ec4a8

Co-authored-by: Weston Ruter <westonruter@google.com>

Ensure amp_remove_endpoint removes repeated /amp/ slugs

aaf0e2e

Extend amp_has_query_var() to look at WP_Query for query vars

ecd9860

Add value of 1 to amp query var added in AMP-to-AMP linking

123a64f

Reuse amp_has_query_var() in AMP_Theme_Support::finish_init()

52a6fbf

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.

westonruter reviewed Oct 5, 2020

View reviewed changes

westonruter reviewed Oct 6, 2020

View reviewed changes

westonruter added 6 commits October 6, 2020 12:03

Rename amp_get_url() to amp_get_paired_url()

b66c233

Assemble CRUD functions for paired AMP endpoints

d5610f2

Skip test_theme_data_exists when Neve already installed

aafcaec

Remove hard-coded dependency on amp=1 query var for AMP-to-AMP linking

04f53d9

Replace usages of amp_remove_endpoint() with amp_remove_paired_endpoi…

ce34a99

…nt()

Use strings for query var values

0931662

westonruter approved these changes Oct 7, 2020

View reviewed changes

westonruter added 2 commits October 9, 2020 16:07

Use amp_has_paired_endpoint() and amp_add_paired_endpoint() naming

0f7a3a9

westonruter merged commit c3aeec5 into develop Oct 10, 2020

westonruter deleted the enhancement/2204-default-amp-endpoint branch October 10, 2020 20:36

westonruter mentioned this pull request Oct 12, 2020

Default to ?amp=1 query param instead of /amp/ endpoint slug in all scenarios (but persist legacy config and allow customization) #2204

Closed

westonruter mentioned this pull request Oct 28, 2020

AMP Helper Function Chooses Not To Append Query Value - ($use_query_var) Evaluates To False #4442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to ?amp=1 query parameter #4312

Default to ?amp=1 query parameter #4312

pierlon commented Feb 18, 2020 •

edited by westonruter

Loading

westonruter commented Feb 24, 2020

pierlon commented Aug 30, 2020

github-actions bot commented Aug 30, 2020 •

edited

Loading

westonruter Sep 5, 2020

westonruter Sep 5, 2020

pierlon Sep 23, 2020

westonruter Oct 5, 2020

westonruter Oct 6, 2020

westonruter Oct 6, 2020

westonruter Oct 6, 2020

westonruter Oct 6, 2020

westonruter Oct 6, 2020 •

edited

Loading

westonruter Oct 6, 2020

pierlon Oct 6, 2020

westonruter Oct 6, 2020

westonruter left a comment

westonruter Oct 7, 2020

pierlon commented Oct 7, 2020

schlessera commented Oct 8, 2020

westonruter commented Oct 8, 2020

schlessera commented Oct 8, 2020

schlessera commented Oct 8, 2020

westonruter commented Oct 8, 2020

schlessera commented Oct 9, 2020 •

edited

Loading

westonruter commented Oct 9, 2020

westonruter commented Oct 9, 2020

westonruter commented Oct 9, 2020

westonruter commented Oct 9, 2020

westonruter commented Oct 10, 2020

	return isset( $_GET[ amp_get_slug() ] ) && 1 === filter_var( $_GET[ amp_get_slug() ], FILTER_VALIDATE_INT ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended
	return isset( $_GET[ amp_get_slug() ] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended

		if ( isset( $path_args[ amp_get_slug() ] ) && '' !== $path_args[ amp_get_slug() ] ) {
		if ( isset( $path_args[ amp_get_slug() ] ) && '1' !== $path_args[ amp_get_slug() ] ) {

Default to ?amp=1 query parameter #4312

Default to ?amp=1 query parameter #4312

Conversation

pierlon commented Feb 18, 2020 • edited by westonruter Loading

Summary

westonruter commented Feb 24, 2020

pierlon commented Aug 30, 2020

github-actions bot commented Aug 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon commented Oct 7, 2020

schlessera commented Oct 8, 2020

westonruter commented Oct 8, 2020

schlessera commented Oct 8, 2020

schlessera commented Oct 8, 2020

westonruter commented Oct 8, 2020

schlessera commented Oct 9, 2020 • edited Loading

westonruter commented Oct 9, 2020

westonruter commented Oct 9, 2020

westonruter commented Oct 9, 2020

westonruter commented Oct 9, 2020

westonruter commented Oct 10, 2020

pierlon commented Feb 18, 2020 •

edited by westonruter

Loading

github-actions bot commented Aug 30, 2020 •

edited

Loading

westonruter Oct 6, 2020 •

edited

Loading

schlessera commented Oct 9, 2020 •

edited

Loading