Skip to content

Commit

Permalink
Go back to using query_var instead of query_param
Browse files Browse the repository at this point in the history
  • Loading branch information
westonruter committed Jan 21, 2021
1 parent ac968be commit 651b411
Show file tree
Hide file tree
Showing 9 changed files with 45 additions and 42 deletions.
12 changes: 6 additions & 6 deletions assets/src/settings-page/paired-url-structure.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { AMPNotice, NOTICE_TYPE_INFO, NOTICE_SIZE_LARGE } from '../components/am

/**
* @typedef {{name: string, slug: string, type: string}} Source
* @typedef {{query_param: string[], path_suffix: string[], legacy_transitional: string[], legacy_reader: string[], custom: string[]}} PairedUrlExamplesData
* @typedef {{query_var: string[], path_suffix: string[], legacy_transitional: string[], legacy_reader: string[], custom: string[]}} PairedUrlExamplesData
*/

/**
Expand Down Expand Up @@ -152,16 +152,16 @@ export function PairedUrlStructure( { focusedSection } ) {
<ul>
<li>
<input
id="paired_url_structure_query_param"
id="paired_url_structure_query_var"
type="radio"
name="paired_url_structure"
checked={ 'query_param' === editedOptions.paired_url_structure }
checked={ 'query_var' === editedOptions.paired_url_structure }
onChange={ () => {
updateOptions( { paired_url_structure: 'query_param' } );
updateOptions( { paired_url_structure: 'query_var' } );
} }
disabled={ isCustom }
/>
<label htmlFor="paired_url_structure_query_param">
<label htmlFor="paired_url_structure_query_var">
{ __( 'Query parameter', 'amp' ) + ': ' }
<code>
{ `?${ slug }=1` }
Expand All @@ -171,7 +171,7 @@ export function PairedUrlStructure( { focusedSection } ) {
{ __( '(recommended)', 'amp' ) }
</em>
</label>
<PairedUrlExamples pairedUrls={ editedOptions.paired_url_examples.query_param } />
<PairedUrlExamples pairedUrls={ editedOptions.paired_url_examples.query_var } />
</li>
<li>
<input
Expand Down
2 changes: 1 addition & 1 deletion includes/options/class-amp-options-manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class AMP_Options_Manager {
Option::SUPPORTED_TEMPLATES => [ 'is_singular' ],
Option::VERSION => AMP__VERSION,
Option::READER_THEME => ReaderThemes::DEFAULT_READER_THEME,
Option::PAIRED_URL_STRUCTURE => Option::PAIRED_URL_STRUCTURE_QUERY_PARAM,
Option::PAIRED_URL_STRUCTURE => Option::PAIRED_URL_STRUCTURE_QUERY_VAR,
Option::PLUGIN_CONFIGURED => false,
];

Expand Down
6 changes: 3 additions & 3 deletions src/Option.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,20 +46,20 @@ interface Option {
/**
* Indicate the structure for paired AMP URLs.
*
* Default value: 'query_param'
* Default value: 'query_var'
*
* @var string
*/
const PAIRED_URL_STRUCTURE = 'paired_url_structure';

/**
* Query param paired URL structure.
* Query var paired URL structure.
*
* This is the default, where all AMP URLs end in `?amp=1`.
*
* @var string
*/
const PAIRED_URL_STRUCTURE_QUERY_PARAM = 'query_param';
const PAIRED_URL_STRUCTURE_QUERY_VAR = 'query_var';

/**
* Path suffix paired URL structure.
Expand Down
10 changes: 5 additions & 5 deletions src/PairedRouting.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
use AmpProject\AmpWP\Admin\ReaderThemes;
use AmpProject\AmpWP\PairedUrlStructure\LegacyReaderUrlStructure;
use AmpProject\AmpWP\PairedUrlStructure\LegacyTransitionalUrlStructure;
use AmpProject\AmpWP\PairedUrlStructure\QueryParamUrlStructure;
use AmpProject\AmpWP\PairedUrlStructure\QueryVarUrlStructure;
use AmpProject\AmpWP\PairedUrlStructure\PathSuffixUrlStructure;
use WP_Query;
use WP_Rewrite;
Expand All @@ -42,7 +42,7 @@ final class PairedRouting implements Service, Registerable {
* @var string[]
*/
const PAIRED_URL_STRUCTURES = [
Option::PAIRED_URL_STRUCTURE_QUERY_PARAM => QueryParamUrlStructure::class,
Option::PAIRED_URL_STRUCTURE_QUERY_VAR => QueryVarUrlStructure::class,
Option::PAIRED_URL_STRUCTURE_PATH_SUFFIX => PathSuffixUrlStructure::class,
Option::PAIRED_URL_STRUCTURE_LEGACY_TRANSITIONAL => LegacyTransitionalUrlStructure::class,
Option::PAIRED_URL_STRUCTURE_LEGACY_READER => LegacyReaderUrlStructure::class,
Expand Down Expand Up @@ -197,7 +197,7 @@ public function get_paired_url_structure() {
if ( array_key_exists( $structure_slug, self::PAIRED_URL_STRUCTURES ) ) {
$structure_class = self::PAIRED_URL_STRUCTURES[ $structure_slug ];
} else {
$structure_class = QueryParamUrlStructure::class;
$structure_class = QueryVarUrlStructure::class;
}
}

Expand Down Expand Up @@ -255,7 +255,7 @@ public function filter_rest_options( $options ) {
$options[ Option::PAIRED_URL_STRUCTURE ] = AMP_Options_Manager::get_option( Option::PAIRED_URL_STRUCTURE );

// Handle edge case where an unrecognized paired URL structure was saved.
if ( ! in_array( $options[ Option::PAIRED_URL_STRUCTURE ], self::PAIRED_URL_STRUCTURES, true ) ) {
if ( ! in_array( $options[ Option::PAIRED_URL_STRUCTURE ], array_keys( self::PAIRED_URL_STRUCTURES ), true ) ) {
$defaults = $this->filter_default_options( [], $options );

$options[ Option::PAIRED_URL_STRUCTURE ] = $defaults[ Option::PAIRED_URL_STRUCTURE ];
Expand Down Expand Up @@ -558,7 +558,7 @@ public function is_using_permalinks() {
* @return array Defaults.
*/
public function filter_default_options( $defaults, $options ) {
$value = Option::PAIRED_URL_STRUCTURE_QUERY_PARAM;
$value = Option::PAIRED_URL_STRUCTURE_QUERY_VAR;

if (
isset( $options[ Option::VERSION ], $options[ Option::THEME_SUPPORT ], $options[ Option::READER_THEME ] )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,14 @@
/**
* Descriptor for paired URL structures that include the `?amp=1` query parameter.
*
* A query parameter is also known as a query arg and a query var. Given prior usage of "query var" in the codebase,
* the slug for the paired URL structure is `query_var` whereas in the UI it is presented as "query parameter".
*
* @package AmpProject\AmpWP
* @since 2.1
* @internal
*/
final class QueryParamUrlStructure extends PairedUrlStructure {
final class QueryVarUrlStructure extends PairedUrlStructure {

/**
* Turn a given URL into a paired AMP URL.
Expand Down
38 changes: 19 additions & 19 deletions tests/php/src/PairedRoutingTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use AmpProject\AmpWP\PairedUrlStructure\LegacyReaderUrlStructure;
use AmpProject\AmpWP\PairedUrlStructure\LegacyTransitionalUrlStructure;
use AmpProject\AmpWP\PairedUrlStructure\PathSuffixUrlStructure;
use AmpProject\AmpWP\PairedUrlStructure\QueryParamUrlStructure;
use AmpProject\AmpWP\PairedUrlStructure\QueryVarUrlStructure;
use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility;
use AMP_Options_Manager;
use AMP_Theme_Support;
Expand Down Expand Up @@ -67,9 +67,9 @@ public function test_register() {
/** @return array */
public function get_data_for_test_get_paired_url_structure() {
return [
'query_param' => [
Option::PAIRED_URL_STRUCTURE_QUERY_PARAM,
QueryParamUrlStructure::class,
'query_var' => [
Option::PAIRED_URL_STRUCTURE_QUERY_VAR,
QueryVarUrlStructure::class,
],
'path_suffix' => [
Option::PAIRED_URL_STRUCTURE_PATH_SUFFIX,
Expand All @@ -85,7 +85,7 @@ public function get_data_for_test_get_paired_url_structure() {
],
'bogus' => [
'bogus',
QueryParamUrlStructure::class,
QueryVarUrlStructure::class,
],
];
}
Expand Down Expand Up @@ -202,15 +202,15 @@ public function test_get_endpoint_path_slug_conflicts() {
/** @return array */
public function get_data_for_test_paired_requests() {
return [
'query_param_reader_mode_amp' => [
'query_var_reader_mode_amp' => [
AMP_Theme_Support::READER_MODE_SLUG,
Option::PAIRED_URL_STRUCTURE_QUERY_PARAM,
Option::PAIRED_URL_STRUCTURE_QUERY_VAR,
'/?amp=1',
true,
],
'query_param_reader_mode_non_amp' => [
'query_var_reader_mode_non_amp' => [
AMP_Theme_Support::READER_MODE_SLUG,
Option::PAIRED_URL_STRUCTURE_QUERY_PARAM,
Option::PAIRED_URL_STRUCTURE_QUERY_VAR,
'/',
false,
],
Expand Down Expand Up @@ -240,7 +240,7 @@ public function get_data_for_test_paired_requests() {
],
'standard_mode' => [
AMP_Theme_Support::STANDARD_MODE_SLUG,
Option::PAIRED_URL_STRUCTURE_QUERY_PARAM,
Option::PAIRED_URL_STRUCTURE_QUERY_VAR,
'/',
null,
],
Expand Down Expand Up @@ -311,8 +311,8 @@ function ( $query_vars ) use ( &$request_uri_during_parse_request ) {
/** @return array */
public function get_data_for_test_initialize_paired_request() {
return [
'query_param' => [
Option::PAIRED_URL_STRUCTURE_QUERY_PARAM,
'query_var' => [
Option::PAIRED_URL_STRUCTURE_QUERY_VAR,
false,
],
'path_suffix' => [
Expand Down Expand Up @@ -351,7 +351,7 @@ public function test_initialize_paired_request( $structure, $filtering_unique_po
/** @covers ::initialize_paired_request() */
public function test_initialize_paired_request_in_standard_mode() {
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG );
AMP_Options_Manager::update_option( Option::PAIRED_URL_STRUCTURE, Option::PAIRED_URL_STRUCTURE_QUERY_PARAM );
AMP_Options_Manager::update_option( Option::PAIRED_URL_STRUCTURE, Option::PAIRED_URL_STRUCTURE_QUERY_VAR );
$this->instance->initialize_paired_request();
$this->assertNull( $this->get_private_property( $this->instance, 'did_request_endpoint' ) );
$this->assertFalse( has_filter( 'do_parse_request', [ $this->instance, 'extract_endpoint_from_environment_before_parse_request' ] ) );
Expand Down Expand Up @@ -468,7 +468,7 @@ public function get_data_for_test_filter_default_options() {
Option::THEME_SUPPORT => AMP_Theme_Support::TRANSITIONAL_MODE_SLUG,
Option::READER_THEME => ReaderThemes::DEFAULT_READER_THEME,
],
Option::PAIRED_URL_STRUCTURE_QUERY_PARAM,
Option::PAIRED_URL_STRUCTURE_QUERY_VAR,
],
'old_version_transitional' => [
[
Expand Down Expand Up @@ -819,10 +819,10 @@ static function ( $url ) use ( &$redirected_url ) {
public function test_redirect_extraneous_paired_endpoint_canonical_extraneous_query_var() {
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::STANDARD_MODE_SLUG );
$this->set_permalink_structure( '/%year%/%monthnum%/%day%/%postname%/' );
$query_param_structure = $this->injector->make( QueryParamUrlStructure::class );
$query_var_structure = $this->injector->make( QueryVarUrlStructure::class );

$permalink_url = get_permalink( self::factory()->post->create() );
$amp_endpoint_url = $query_param_structure->add_endpoint( $permalink_url );
$amp_endpoint_url = $query_var_structure->add_endpoint( $permalink_url );
$this->go_to( $amp_endpoint_url );

$this->assertTrue( amp_is_canonical() );
Expand All @@ -843,7 +843,7 @@ static function ( $url ) use ( &$redirected_url ) {
/** @covers ::redirect_extraneous_paired_endpoint() */
public function test_redirect_extraneous_paired_endpoint_path_suffix_404() {
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG );
AMP_Options_Manager::update_option( Option::PAIRED_URL_STRUCTURE, Option::PAIRED_URL_STRUCTURE_QUERY_PARAM );
AMP_Options_Manager::update_option( Option::PAIRED_URL_STRUCTURE, Option::PAIRED_URL_STRUCTURE_QUERY_VAR );
$this->set_permalink_structure( '/%year%/%monthnum%/%day%/%postname%/' );
$path_suffix_structure = $this->injector->make( PathSuffixUrlStructure::class );
$paired_url = $this->injector->make( PairedUrl::class );
Expand Down Expand Up @@ -873,7 +873,7 @@ static function ( $url ) use ( &$redirected_url ) {
/** @covers ::redirect_extraneous_paired_endpoint() */
public function test_redirect_extraneous_paired_endpoint_slug_redirect() {
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG );
AMP_Options_Manager::update_option( Option::PAIRED_URL_STRUCTURE, Option::PAIRED_URL_STRUCTURE_QUERY_PARAM );
AMP_Options_Manager::update_option( Option::PAIRED_URL_STRUCTURE, Option::PAIRED_URL_STRUCTURE_QUERY_VAR );
AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false );
AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ 'is_singular' ] );
$this->set_permalink_structure( '/%year%/%monthnum%/%day%/%postname%/' );
Expand Down Expand Up @@ -918,7 +918,7 @@ static function ( $url ) {
/** @covers ::redirect_extraneous_paired_endpoint() */
public function test_redirect_extraneous_paired_endpoint_unavailable_template() {
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG );
AMP_Options_Manager::update_option( Option::PAIRED_URL_STRUCTURE, Option::PAIRED_URL_STRUCTURE_QUERY_PARAM );
AMP_Options_Manager::update_option( Option::PAIRED_URL_STRUCTURE, Option::PAIRED_URL_STRUCTURE_QUERY_VAR );
AMP_Options_Manager::update_option( Option::ALL_TEMPLATES_SUPPORTED, false );
AMP_Options_Manager::update_option( Option::SUPPORTED_TEMPLATES, [ 'is_singular' ] );
$this->set_permalink_structure( '/%year%/%monthnum%/%day%/%postname%/' );
Expand Down
8 changes: 4 additions & 4 deletions tests/php/src/PairedUrlStructure/QueryVarUrlStructureTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,18 @@

namespace AmpProject\AmpWP\Tests\PairedUrlStructure;

use AmpProject\AmpWP\PairedUrlStructure\QueryParamUrlStructure;
use AmpProject\AmpWP\PairedUrlStructure\QueryVarUrlStructure;
use AmpProject\AmpWP\Tests\DependencyInjectedTestCase;

/** @coversDefaultClass \AmpProject\AmpWP\PairedUrlStructure\QueryParamUrlStructure */
/** @coversDefaultClass \AmpProject\AmpWP\PairedUrlStructure\QueryVarUrlStructure */
class QueryVarUrlStructureTest extends DependencyInjectedTestCase {

/** @var QueryParamUrlStructure */
/** @var QueryVarUrlStructure */
private $instance;

public function setUp() {
parent::setUp();
$this->instance = $this->injector->make( QueryParamUrlStructure::class );
$this->instance = $this->injector->make( QueryVarUrlStructure::class );
}

/** @covers ::add_endpoint() */
Expand Down
4 changes: 2 additions & 2 deletions tests/php/test-amp-helper-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -1849,7 +1849,7 @@ public function test_amp_add_paired_endpoint() {
public function data_amp_has_paired_endpoint() {
return [
'nothing' => [
Option::PAIRED_URL_STRUCTURE_QUERY_PARAM,
Option::PAIRED_URL_STRUCTURE_QUERY_VAR,
'',
false,
],
Expand All @@ -1859,7 +1859,7 @@ public function data_amp_has_paired_endpoint() {
true,
],
'url_param_value' => [
Option::PAIRED_URL_STRUCTURE_QUERY_PARAM,
Option::PAIRED_URL_STRUCTURE_QUERY_VAR,
'?amp=1',
true,
],
Expand Down
2 changes: 1 addition & 1 deletion tests/php/test-class-amp-options-manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public function test_get_and_set_options() {
Option::MOBILE_REDIRECT => false,
Option::READER_THEME => 'legacy',
Option::PLUGIN_CONFIGURED => false,
Option::PAIRED_URL_STRUCTURE => Option::PAIRED_URL_STRUCTURE_QUERY_PARAM,
Option::PAIRED_URL_STRUCTURE => Option::PAIRED_URL_STRUCTURE_QUERY_VAR,
],
AMP_Options_Manager::get_options()
);
Expand Down

0 comments on commit 651b411

Please sign in to comment.