Add `amp option` wp cli command to manage plugin options by thelovekesh · Pull Request #7430 · ampproject/amp-wp

thelovekesh · 2023-01-30T14:13:43Z

Summary

Fixes #5783
Previously #7368

Checklist

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).

github-actions · 2023-01-30T14:18:38Z

Plugin builds for 68bc2ac are ready 🛎️!

Download development build
Download production build

westonruter

Just pulling in this previous PR review: #7368 (review)

src/Cli/OptionCommand.php

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

src/Cli/OptionCommand.php

westonruter

Code looks good! Just a few more bits of feedback.

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

* Remove .babelrc, .eslintrc files and add *.feature file extension

… options to be managed by amp option command

…via CLI

… guard in update subcommand

…ion subcommands

…ome options

westonruter · 2023-02-03T17:54:33Z

src/Cli/OptionCommand.php

+		// Update type for some options.
+		if ( Option::SANDBOXING_LEVEL === $option_name ) {
+			$option_value = (int) $option_value;
+		}


Why was this specific cast needed? Is it because it is a string and the following logic requires it to be an int:

amp-wp/src/Sandboxing.php

Lines 123 to 129 in c28b8f1

if (

isset( $new_options[ Option::SANDBOXING_LEVEL ] )

&&

in_array( $new_options[ Option::SANDBOXING_LEVEL ], self::LEVELS, true )

) {

$options[ Option::SANDBOXING_LEVEL ] = $new_options[ Option::SANDBOXING_LEVEL ];

}

It seems like the Sandboxing service should be doing that cast to int as well, since it is already doing so for bool:

amp-wp/src/Sandboxing.php

Lines 120 to 122 in c28b8f1

if ( isset( $new_options[ Option::SANDBOXING_ENABLED ] ) ) {

$options[ Option::SANDBOXING_ENABLED ] = (bool) $new_options[ Option::SANDBOXING_ENABLED ];

}

I believe when we update options using REST API then it first validates the options(and return any validation failures) through provided schema. In the case of '0' and '1' and 'true' and 'false' with schema type as boolean, it runs it through the rest_sanitize_boolean first hence type cast is not required but in the case of enums it fails:

amp-wp/src/Sandboxing.php

Line 56 in c28b8f1

const LEVELS = [ 1, 2, 3 ];

amp-wp/src/Sandboxing.php

Lines 68 to 72 in c28b8f1

Option::SANDBOXING_LEVEL => [

'type' => 'int',

'enum' => self::LEVELS,

'default' => 1,

],

In other words, it doesn't reach here:

amp-wp/includes/options/class-amp-options-manager.php

Line 368 in c28b8f1

$options = apply_filters( 'amp_options_updating', $options, $new_options );

It seems like the Sandboxing service should be doing that cast to int as well since it is already doing so for bool:

Type cast here is done by REST schema's rest_sanitize_boolean IMO.

thelovekesh added 8 commits December 6, 2022 22:34

Add initial fixture for AMP options command

a6af8ec

Add AMP Options cli command in services

5398f88

[skip ci] Merge branch 'develop' into add/plugin-settings-wp-cli-command

fbfcb62

Add method to make REST requests

1dbcdaf

Remove allowed_args schema

708a9ac

Update get, list and update commands to use REST API

3eca69d

Merge branch 'develop' into add/plugin-settings-wp-cli-command

8cab1f3

Merge branch 'develop' into add/plugin-settings-wp-cli-command

e56ff1a

thelovekesh changed the title ~~Add amp option wp cli command to manage amp settings~~ Add amp option wp cli command to manage plugin options Jan 30, 2023

thelovekesh self-assigned this Jan 30, 2023

thelovekesh added Enhancement New feature or improvement of an existing one WP-CLI WS:Core Work stream for Plugin core labels Jan 30, 2023

thelovekesh added this to the v2.4 milestone Jan 30, 2023

Remove Plugin Suppression from amp option command

770536b

westonruter requested changes Jan 30, 2023

View reviewed changes

src/Cli/OptionCommand.php Outdated Show resolved Hide resolved

src/Cli/OptionCommand.php Outdated Show resolved Hide resolved

src/Cli/OptionCommand.php Outdated Show resolved Hide resolved

thelovekesh and others added 13 commits January 31, 2023 14:24

Fix indentation

55818b6

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

Merge branch 'develop' into add/plugin-settings-wp-cli-command

2b32e8d

Update help messages for get command

8764680

Update help messages for update command

f681c68

Update help messages for list command

00f7c3f

Add halt with 0 as exit code when using subcommands in list command

99ec18c

Update WP_Error message when user is not allowed to manage_options

ac7e682

Add list command in amp option command examples

89ee9d5

Update logic to check if option exists in amp option update command

7b9f2ae

Add feature tests for amp option command

3898e0b

Update feature test for amp option list command

5c1cb94

Removed test cases which are causing STDERR

9bcf2e4

Add feature tests for amp option command with STDERR results

d23d8bb

westonruter reviewed Feb 2, 2023

View reviewed changes

src/Cli/OptionCommand.php Outdated Show resolved Hide resolved

westonruter reviewed Feb 2, 2023

View reviewed changes

src/Cli/OptionCommand.php Outdated Show resolved Hide resolved

westonruter requested changes Feb 2, 2023

View reviewed changes

Use namespace alias to import classes instead of \ [skip ci]

0afc408

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

thelovekesh force-pushed the add/plugin-settings-wp-cli-command branch from 8e82e22 to 0afc408 Compare February 2, 2023 19:22

thelovekesh added 18 commits February 3, 2023 00:53

Merge branch 'develop' into add/plugin-settings-wp-cli-command [skip ci]

b3d40d1

Revert changes done in d467edd [skip ci]

de7a54c

* Remove .babelrc, .eslintrc files and add *.feature file extension

Import required classes using namespace aliasing

d4dcedc

Add sandboxing_enabled, sandboxing_level and delete_data_at_uninstall…

e9c4cc4

… options to be managed by amp option command

Update amp option list command to print options which can be updated …

d721bbc

…via CLI

Add new subcommand list-reader-themes for amp option command

ba90ffd

Refactor list subcommand to show only cli-managed amp options

9b7e487

Add feature tests for amp option list-reader-themes command

dcb78ac

Update list subcommand to not STDOUT notices in case of pipe the output

099e1a7

Add isPiped function in static analysis stubs for wp-cli

1517d3d

Update notices in list subcommand

64845f8

Remove guard to check if option exist for the sake of ALLOWED_OPTIONS…

dfc5680

… guard in update subcommand

Update message string in update subcommand when could not update options

a2210b4

Update feature tests for amp option update command

39f9605

Update string to keep option_name in quotes in amp option subcommands

5a30736

Update feature tests after including option_name in quotes in amp opt…

76fe5b6

…ion subcommands

Update list command feature tests after refactoring in list subcommand

4719982

Update the update subcommand to handle type casting before updating s…

379455f

…ome options

thelovekesh requested a review from westonruter February 3, 2023 00:22

Fix typo in feature tests for option command

68bc2ac

westonruter reviewed Feb 3, 2023

View reviewed changes

westonruter approved these changes Feb 3, 2023

View reviewed changes

westonruter merged commit 68d9bb3 into develop Feb 3, 2023

westonruter deleted the add/plugin-settings-wp-cli-command branch February 3, 2023 18:20

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `amp option` wp cli command to manage plugin options#7430

Add `amp option` wp cli command to manage plugin options#7430
westonruter merged 59 commits intodevelopfrom
add/plugin-settings-wp-cli-command

thelovekesh commented Jan 30, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Jan 30, 2023 •

edited

Loading

Uh oh!

westonruter left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westonruter left a comment

Uh oh!

westonruter Feb 3, 2023

Uh oh!

thelovekesh Feb 3, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if (
	isset( $new_options[ Option::SANDBOXING_LEVEL ] )
	&&
	in_array( $new_options[ Option::SANDBOXING_LEVEL ], self::LEVELS, true )
	) {
	$options[ Option::SANDBOXING_LEVEL ] = $new_options[ Option::SANDBOXING_LEVEL ];
	}

	if ( isset( $new_options[ Option::SANDBOXING_ENABLED ] ) ) {
	$options[ Option::SANDBOXING_ENABLED ] = (bool) $new_options[ Option::SANDBOXING_ENABLED ];
	}

	Option::SANDBOXING_LEVEL => [
	'type' => 'int',
	'enum' => self::LEVELS,
	'default' => 1,
	],

Conversation

thelovekesh commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

github-actions bot commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

westonruter Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

thelovekesh Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thelovekesh commented Jan 30, 2023 •

edited

Loading

github-actions bot commented Jan 30, 2023 •

edited

Loading

thelovekesh Feb 3, 2023 •

edited

Loading