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

Prevent empty query var from causing false positive endpoint detection #5804

Merged
merged 2 commits into from
Jan 25, 2021

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jan 24, 2021

Amends #5558. See #2204.

Thanks to @milindmore22 for pointing this out, there's a bug with in how the AMP endpoints are detected. In particular, when a non-AMP URL is request with an empty query var, e.g. ?foo=, the result is the URL erroneously being identified as an AMP one. The reason for this is that I was removing the endpoint from the URL and then checking of the endpoint-removed URL was not equal to the original URL as a shortcut to identify an AMP URL. This, however, doesn't work because remove_query_var() doesn't preserve the distinction between a query var with an empty value and one with no value:

wp> remove_query_arg( 'foo', '?bar=' )
=> string(4) "?bar"
wp> remove_query_arg( 'foo', '?bar' )
=> string(4) "?bar"

Notice how ?bar= and ?bar both end up the same as ?bar when being passed into remove_query_arg().

So the fix is to check if the URL has the endpoint first, and then if it has it, proceed with removing it. This avoids the false positive.

@westonruter westonruter added this to the v2.1 milestone Jan 24, 2021
@codecov
Copy link

codecov bot commented Jan 24, 2021

Codecov Report

Merging #5804 (6770c1b) into develop (4890db0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #5804   +/-   ##
==========================================
  Coverage      75.12%   75.12%           
  Complexity      5645     5645           
==========================================
  Files            210      210           
  Lines          16954    16955    +1     
==========================================
+ Hits           12736    12737    +1     
  Misses          4218     4218           
Flag Coverage Δ Complexity Δ
javascript 75.89% <ø> (ø) 0.00 <ø> (ø)
php 75.09% <100.00%> (+<0.01%) 0.00 <0.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
src/PairedRouting.php 96.01% <100.00%> (+0.01%) 111.00 <0.00> (ø)

@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2021

Plugin builds for 6770c1b are ready 🛎️!

src/PairedRouting.php Outdated Show resolved Hide resolved
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
@westonruter westonruter merged commit 71d9d4f into develop Jan 25, 2021
@westonruter westonruter deleted the fix/empty-query-var-causing-amp-endpoint branch January 25, 2021 04:26
@westonruter westonruter self-assigned this Apr 25, 2021
@westonruter
Copy link
Member Author

QA Passed

Going to /sample-page/?foo=1 does not serve an AMP page whereas going to /sample-page/?amp=1 does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants