-
Notifications
You must be signed in to change notification settings - Fork 25
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 pluck/patch commands for caches and transients #89
base: main
Are you sure you want to change the base?
Add pluck/patch commands for caches and transients #89
Conversation
8787b05
to
f31a3dd
Compare
I cannot reproduce the failures for the functional tests locally :
Seem's like the mu-plugins used to set up those scenarios are not loaded. |
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 cannot reproduce the failures for the functional tests locally :
Yeah, the tests pass locally for me too.
I don't know what the issue might be. @wp-cli/committers Any ideas?
Unfortunately can't reproduce locally either. Looks like Adding some |
Scratch that, it was an issue with the rebase 🥲 |
4f765d0
to
6e5ecdf
Compare
@danielbachhuber I've split the tests into dedicated files and update them to use In 327d99a I added two debug statement to try to understand why the tests are failing in Github CI. |
6cd56d8
to
e75e7c1
Compare
@danielbachhuber I was finally able to fix the failures for the functional tests. It was related to the use of STDIN to get the new value, which didn't properly handle the case where it was empty. All tests are green on my fork. |
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.
Nice work getting the tests working, @petitphp !
I left a few comments to work through.
features/transient-patch.feature
Outdated
set_transient( 'my_key_2', ['foo' => ['bar' => 'baz']] ); | ||
}; | ||
|
||
WP_CLI::add_hook( 'before_invoke:transient patch', $set_foo ); |
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.
Can we use wp transient set
to set these initial transient values? The transient cache is persistent in vanilla WordPress, so we shouldn't need to hook into the command invocation in this manner.
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.
Originally I was planing on using wp transient set
but it is not possible to set array-like data directly like what's possible with wp option add/update
(maybe an idea for a future PR to add this).
I've found another solution by using the wp eval
command to execute a set_transient
Given a WP install
And I run `wp eval "set_transient( 'my_key', ['foo' => 'bar'] );"`
And I run `wp eval "set_transient( 'my_key_2', ['foo' => ['bar' => 'baz']] );"`
$traverser = new RecursiveDataStructureTraverser( $current_value ); | ||
|
||
try { | ||
$traverser->$action( $key_path, $patch_value ); |
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.
How does the actual transient value get saved?
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.
The transient value is updated later in the elseif
condition by using the function store in the $write_func
variable.
What's left to do on this PR? |
Hi @johnbillion, sorry for the delay. I didn't had the time to look at Daniel comments yet. I'll try to check them this week and finish up this PR for a new review. |
Hi @danielbachhuber, I've refactored the feature tests to address your feedbacks, this should be good to go. All tests workflows using MySQL are green, the ones with SQLite are failing, but doesn't seem related to this PR specifically (eg: Functional - WP latest on 8.2 with SQLite). |
SQLite errors are known, see #92 |
Hi, this PR should be ready for final review, the failling tests are related to SQLite errors and not to the changes in this PR. |
b9d6f5e
to
d8ef33a
Compare
Split features for patch/pluck commands into their own file and use `When I run` for commands when they have a successful outcome.
Co-authored-by: Daniel Bachhuber <daniel@bachhuber.co>
Co-authored-by: Daniel Bachhuber <daniel@bachhuber.co>
Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com>
Co-authored-by: Pascal Birchler <pascal.birchler@gmail.com>
44903e8
to
9eca4d9
Compare
Add commands for plucking and patching values from object cache and transient.
Fixes #26