Skip to content

Conversation

@thelovekesh
Copy link
Collaborator

Summary

Fixes #5783

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

@thelovekesh thelovekesh self-assigned this Dec 11, 2022
@thelovekesh thelovekesh force-pushed the add/plugin-settings-wp-cli-command branch from d9f50af to fbfcb62 Compare December 12, 2022 07:18
@thelovekesh thelovekesh force-pushed the add/plugin-settings-wp-cli-command branch from f8e212b to fbfcb62 Compare December 20, 2022 06:09
@westonruter westonruter added this to the v2.3.1 milestone Jan 12, 2023
Option::READER_THEME,
Option::THEME_SUPPORT,
Option::MOBILE_REDIRECT,
Option::SUPPRESSED_PLUGINS,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we don't need to include Suppressed Plugins

Suggested change
Option::SUPPRESSED_PLUGINS,

Comment on lines 58 to 64

/**
* PluginSuppression instance.
*
* @var PluginSuppression
*/
private $plugin_suppression;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/**
* PluginSuppression instance.
*
* @var PluginSuppression
*/
private $plugin_suppression;

Comment on lines 79 to 83
* @param PluginSuppression $plugin_suppression PluginSuppression instance.
*/
public function __construct( ReaderThemes $reader_themes, PluginSuppression $plugin_suppression ) {
$this->reader_themes = $reader_themes;
$this->plugin_suppression = $plugin_suppression;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param PluginSuppression $plugin_suppression PluginSuppression instance.
*/
public function __construct( ReaderThemes $reader_themes, PluginSuppression $plugin_suppression ) {
$this->reader_themes = $reader_themes;
$this->plugin_suppression = $plugin_suppression;
*/
public function __construct( ReaderThemes $reader_themes ) {
$this->reader_themes = $reader_themes;

Comment on lines 162 to 163
// Add reader themes to the options.
$options[ self::READER_THEMES ] = wp_list_pluck( $this->reader_themes->get_themes(), 'slug' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be confusing to a user as this is not actually an option.

If we want a way to list the available Reader themes, this can be done via a separate command entirely. For example, wp amp options list-reader-themes.

@westonruter westonruter modified the milestones: v2.3.1, v2.4 Jan 26, 2023
@thelovekesh thelovekesh force-pushed the add/plugin-settings-wp-cli-command branch from 8cab1f3 to bbc5c1c Compare January 30, 2023 13:50
@westonruter westonruter removed this from the v2.4 milestone Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add WP-CLI commands for managing AMP settings

3 participants