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

Add wp config is-true command #161

Merged
merged 21 commits into from
Aug 24, 2023

Conversation

rwagner00
Copy link
Contributor

@rwagner00 rwagner00 commented Aug 21, 2023

Resolves #159 by providing config is-true command along with related automated tests.

It creates a protected method get_value() to prevent code duplication between the existing get() and new is_true() methods.

Contributors: @pwtyler @rwagner00 @schlessera

Co-authored-by: Phil Tyler <philip@tylerdigital.com>
@jmslbam
Copy link
Contributor

jmslbam commented Aug 22, 2023

As this proces was quit interesting to follow 😁, here is the Slack thread to optimize the running time of the Behat test
https://wordpress.slack.com/archives/C02RP4T41/p1692652486279629

@danielbachhuber danielbachhuber changed the title Is-True Command Aug 24, 2023
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.

Great start! Just a few comments to chat through. It seems generally solid otherwise.

features/config-is-true.feature Outdated Show resolved Hide resolved
* - constant
* - variable
* - all
* ---
Copy link
Member

Choose a reason for hiding this comment

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

Should we offer a --strict flag too, for strict checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm legitimately not sure what could be made stricter than it already is.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, to clarify, --strict would only WP_CLI::halt( 0 ); for define( 'MULTISITE', true );. It would WP_CLI::halt( 1 ); for any truthy value (e.g. define( 'MULTISITE', 'true' );).

We can also leave things as they are, and wait for that to come up as a request.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know that that's particularly valuable until someone asks for it. true/false and 0/1 will already be mostly interchangeable depending on programmer's habit.

src/Config_Command.php Outdated Show resolved Hide resolved
src/Config_Command.php Show resolved Hide resolved
@rwagner00
Copy link
Contributor Author

@danielbachhuber Ready for re-review at your convience.

@danielbachhuber danielbachhuber added this to the 2.3.0 milestone Aug 24, 2023
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.

Looks great! Thanks for your work on this, @rwagner00

@danielbachhuber danielbachhuber merged commit 0940447 into wp-cli:main Aug 24, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment