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

Adds Default options in wp_options table to allow manipulating options from WP CLI. #354

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Vedant-Gandhi
Copy link

Description

The aim of this PR is to add support for purging AMP URL's.

Type

  • New Feature

Reference Issue

Testing

The code has been tested locally and is working as expected.

@Vedant-Gandhi Vedant-Gandhi marked this pull request as ready for review September 19, 2024 06:55
$options = get_site_option( 'rt_wp_nginx_helper_options', array() );
$default_settings = $this->nginx_helper_default_settings();

$removable_default_settings = array(
Copy link

Choose a reason for hiding this comment

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

Why are we removing these settings from the list of settings to be stored in the options table?

Copy link
Author

Choose a reason for hiding this comment

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

@SH4LIN These are redis configurations that if stored by default are populating the redis input fields, but on fresh install the nginx_cache purge method is selected which is causing inconsistencies in behaviour.

unset( $default_settings[ $removable_key ] );
}

$diffed_options = wp_parse_args( $default_settings, $options );
Copy link

Choose a reason for hiding this comment

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

According to the documentation and function signature, keep the default options as the second argument and the options ($args) as the first.

Copy link

@SH4LIN SH4LIN left a comment

Choose a reason for hiding this comment

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

LGTM!

@SH4LIN
Copy link

SH4LIN commented Sep 30, 2024

@Vedant-Gandhi Please check why test is failing before merging.

@Vedant-Gandhi
Copy link
Author

Vedant-Gandhi commented Sep 30, 2024

@SH4LIN The action is throwing an error because it is utilising a deprecated version of the upload artifact API.

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