-
Notifications
You must be signed in to change notification settings - Fork 975
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
Conversation
…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().
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 |
There was a problem hiding this 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.
Hi @danielbachhuber! We had written a script for both single and multisite installs which allowed overriding the site URL with WP-CLI:
When tested on a single-site, we quickly saw: After reviewing the code I felt the two functions should behave consistently. I could technically live with both using In #5353, I noted that it is perhaps a different bug in that |
@dougaxe1 Thanks for the explanation! Seems reasonable 😊 |
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.