Fix AMP version comment pages URL for legacy paired URL structure. #6389
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Plugin builds for d769474 are ready 🛎️!
|
pierlon
suggested changes
Jun 16, 2021
There was a problem hiding this comment.
Great work @dhaval-parekh! Just a few minor changes to be made.
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>
westonruter
reviewed
Jun 16, 2021
westonruter
reviewed
Jun 16, 2021
westonruter
reviewed
Jun 16, 2021
…tion to legacy reader
…for cpage query var
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…legacy-reader-paired-url * 'develop' of github.com:ampproject/amp-wp: (56 commits) Fix path regex for QA plugin workflow Fix script path Fix syntax error Also apply modified paths restrictions for QA tester plugin workflow Fix patterns Only run jobs when related files are modified Rename step Update Gutenberg package dependencies Omit Bento from components when version is not valid Update spec to include ampproject/amphtml#34769 Update Gutenberg package dependencies Set null state Require polyfill directly Replace deprecated `@babel/polyfill` with `@wordpress/babel-preset-default/build/polyfill` Parse result into array Close latest Gutenberg dependency update PR if it is outdated Enable auto merge if needed Remove dependency approve list Switch to `google-github-actions/setup-gcloud` action Fix lint error ...
…redirect_canonical filters have fixed URL
…legacy-reader-paired-url * 'develop' of github.com:ampproject/amp-wp: Bump grunt from 1.4.0 to 1.4.1 in /qa-tester Bump codecov/codecov-action from 1.5.0 to 1.5.2 Bump cweagans/composer-patches from 1.7.0 to 1.7.1 Bump @wordpress/eslint-plugin from 9.0.5 to 9.0.6 in /qa-tester Bump eslint from 7.26.0 to 7.28.0 Bump grunt from 1.4.0 to 1.4.1 Bump php-stubs/wordpress-stubs from 5.7.1 to 5.7.2 Bump google/cloud-storage from 1.23.1 to 1.23.2 Bump postcss-loader from 4.2.0 to 4.3.0
westonruter
reviewed
Jun 17, 2021
Comment on lines
-28
to
+31
| $post_id = $this->url_to_postid( $url ); | ||
| // Make sure any existing AMP endpoint is removed. | ||
| $url = $this->remove_endpoint( $url ); | ||
|
|
||
| $post_id = $this->url_to_postid( $url ); |
There was a problem hiding this comment.
Previously when an AMP endpoint was being supplied, $post_id would always be 0 since it wouldn't be recognized. So removing the endpoint first ensures that url_to_postid works as expected.
There was a problem hiding this comment.
I've added the missing assertions which would have caught this in 28b3fbf.
westonruter
force-pushed
the
bug/6355-legacy-reader-paired-url
branch
from
c7c9d03
to
28b3fbf
Compare
westonruter
reviewed
Jun 17, 2021
Comment on lines
+68
to
+73
| $this->assertEquals( | ||
| trailingslashit( trailingslashit( $post_permalink_url ) . $slug ), | ||
| $this->instance->add_endpoint( trailingslashit( $post_permalink_url ) . $slug ) | ||
| ); | ||
| $this->assertEquals( 2, $amp_pre_get_permalink_count ); | ||
| $this->assertEquals( 2, $amp_get_permalink_count ); |
There was a problem hiding this comment.
Before acf8a17 these assertions would fail.
westonruter
approved these changes
Jun 17, 2021
pierlon
approved these changes
Jun 17, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
To address the issue of invalid redirect for the AMP version comment page for legacy paired URL structure.
Fixes #6355
Checklist