Fix cli cache prune command #5660
Conversation
Use version number instead of timestamp to keep the latest version of each cached package.
|
@bret-miller I send a PR with code style fixes bret-miller#1 |
|
@wojsmol Thank you. I understand better the coding style now. |
|
@bret-miller You can apply changes by merging bret-miller#1 |
Fix code style for wp-cli/wp-cli/wp-cli#5660
Still trying to figure out when to align equal signs and when not to... Apparently this one shouldn't be aligned with the two the follow.
|
This tests failure is related to your PR. |
danielbachhuber
left a comment
danielbachhuber
left a comment
There was a problem hiding this comment.
Thanks for the pull request, @bret-miller !
It's a little confusing to figure out what's going on with the diff.
Could you update your pull request description with more detail on the before and after behaviors?
Alternatively or in addition to, would it be possible to incorporate some test coverage documenting the change?
|
Fixed description at the top. |
@bret-miller Thank you! I'll test this out when I have some time in the next week or so. |
Move comment below else so it can be properly indented.
danielbachhuber
left a comment
danielbachhuber
left a comment
There was a problem hiding this comment.
In looking at this a little further, I think we should change the approach slightly.
public function prune( $method = 'timestamp' )More specifically:
- We'd default to 'timestamp' to avoid a breaking change on the existing behavior.
- Your new behavior could be applied with
$method = 'version'.
Then, we could offer a flag in wp cli cache prune to prune based on 'version' instead of 'timestamp'. I'm not sure if we should change the default behavior, however.
danielbachhuber
added
command:cache
command:cli-cache
command:cli-cache-prune
and removed
command:cache
labels
|
@danielbachhuber, since the current approach to prune removes nothing in the cache since nothing in the cache uses a timestamp anymore, at least as far as I can tell. So your solution would require people to enter another parameter beyond the "wp cli cache prune" command in order to actually prune the cache. Perhaps a modification that uses either the timestamp or a version number based on what it finds in the cache would be a solution? I am just trying to fix a command that is currently completely broken from a functional standpoint. I think it would be confusing for wp cli users to need to add an additional parameter to make a command work when it should just work. It broke when wp cli changed it's approach to suffix downloaded modules with a version number instead of a timestamp. Does it ever use a timestamp anymore? If not, why keep a default that renders a command ineffective? |
|
@bret-miller Ok, I'm amenable to changing However, the change should be applied to |
|
Unfortunately I don't have the time to work on this. So I am pulling back on this for now. Hopefully someone will fix it. |
|
@bret-miller Sounds good! Appreciate the time you put into the pull request. It will serve as a good point of reference for whomever picks it up. |
3 participants
Use version number instead of timestamp to keep the latest version of each cached package.
Prior: prune command did nothing because it was looking for a timestamp suffix for each package which was replaced at some point by a version number.
After: prune command uses highest version number to keep latest package version for each page, removing the rest.
See issue #5467