You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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().
…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 $runner->init_config();, please provide feedback and I can yank the test, or amend.
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.
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.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.