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

Use has_config() in get_config() to prevent warnings on null values #5880

Merged
merged 3 commits into from Dec 13, 2023

Conversation

dougaxe1
Copy link
Contributor

In get_config(), use has_config() to test for key existence instead of equivalent isset() which throws a warning for null values.

A null value can occur if the option is present, but without a value. Certain config values, such as --url, default to null causing has_config() to be true, while throwing a warning in get_config(). This change provides consistency between has_config() and get_config().

Fixes #5353.

…f equivelent isset() which throws a warning for null values.

A null value can occur if the option is present, but without a value. Certain config values, such as --url, default to null causing has_config() to be true, while throwing a warning in get_config(). This change provides consistency between has_config() and get_config().
@dougaxe1 dougaxe1 requested a review from a team as a code owner November 21, 2023 23:36
@dougaxe1
Copy link
Contributor Author

I was unsure whether the test should go in test-configurator.php, or another file -- I don't see any tests for has/get_config currently. I am also unsure whether it is safe for downstream tests to run $runner->init_config();, please provide feedback and I can yank the test, or amend.

Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

I was unsure whether the test should go in test-configurator.php, or another file -- I don't see any tests for has/get_config currently. I am also unsure whether it is safe for downstream tests to run $runner->init_config();, please provide feedback and I can yank the test, or amend.

I think what you have is fine. The tests pass 😁

I'm having a little bit of a hard time grokking the nature of the change, though. Can you spell it out for me?

Also, I'd be curious to hear how you ran across this.

There's probably technically a back compat break here, but it's probably fine to make in practice.

@dougaxe1
Copy link
Contributor Author

Hi @danielbachhuber!

We had written a script for both single and multisite installs which allowed overriding the site URL with WP-CLI:

if ( defined( 'WP_CLI' ) && WP_CLI && WP_CLI::has_config( 'url' ) ) {
	$siteurl = WP_CLI::get_config( 'url' );
}

When tested on a single-site, we quickly saw: Warning: Unknown config option 'url'. Because for url = null, array_key_exists = true but isset = false.

After reviewing the code I felt the two functions should behave consistently. I could technically live with both using isset(), but array_key_exists() is my expectation.

In #5353, I noted that it is perhaps a different bug in that config['url'] exists on all requests regardless of the --url flag existing, but this seems like a simpler fix.

@danielbachhuber
Copy link
Member

@dougaxe1 Thanks for the explanation! Seems reasonable 😊

@danielbachhuber danielbachhuber merged commit 7038788 into wp-cli:main Dec 13, 2023
65 checks passed
@dougaxe1 dougaxe1 deleted the fix/5353-gethas-config branch December 13, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WP_CLI::has_config() vs. WP_CLI::get_config()
2 participants