Skip to content

show warning when it couldn't make cache dir#4456

Merged
gitlost merged 8 commits intomasterfrom
patch-warning
Oct 20, 2017
Merged

show warning when it couldn't make cache dir#4456
gitlost merged 8 commits intomasterfrom
patch-warning

Conversation

@miya0001
Copy link
Member

@miya0001 miya0001 commented Oct 20, 2017

If wp core download has an error to make cache directory by some reason, warning is not useful for us.

$ wp core download
Warning: mkdir(): Permission denied in phar:///tmp/wp-cli.phar/php/WP_CLI/FileCache.php on line 265
md5 hash verified: 2e8744a702a3d9527782d9135a4c9544
Success: WordPress downloaded.

This PR will try to improve this warning like following.

$ wp core download
Downloading WordPress 4.8.2 (en_US)...
md5 hash verified: 2e8744a702a3d9527782d9135a4c9544
Warning: You don't have permission to make directory at "/Users/miyauchi/.wp-cli/cache/core".
Success: WordPress downloaded.

@gitlost
Copy link
Contributor

gitlost commented Oct 20, 2017

It'd be good to use error_get_last() also like in https://github.com/wp-cli/core-command/blob/master/src/Core_Command.php#L138

@miya0001 miya0001 requested a review from a team October 20, 2017 14:00
@gitlost
Copy link
Contributor

gitlost commented Oct 20, 2017

I'm thinking that the file_exists() check before it https://github.com/wp-cli/wp-cli/blob/master/php/WP_CLI/FileCache.php#L261-L264 should be removed and just let mkdir() fail.

Also could you do a phpunit test?! Not sure what directory value to use to make it fail. Maybe just touch() it so it's a file and then with the file_exists() check gone it will fail. Not great. (For an example of testing for warning output see https://github.com/wp-cli/wp-cli/blob/master/tests/test-utils.php#L421)

@miya0001
Copy link
Member Author

miya0001 commented Oct 20, 2017

@gitlost OK, thanks 😊

@miya0001
Copy link
Member Author

miya0001 commented Oct 20, 2017

@gitlost It is finished! 😊

gitlost
gitlost approved these changes Oct 20, 2017
View reviewed changes
Copy link
Contributor

@gitlost gitlost left a comment

Choose a reason for hiding this comment

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

Nice tests! Just pushed some clean up stuff.

@gitlost gitlost added this to the 1.5.0 milestone Oct 20, 2017
@gitlost gitlost merged commit 0f3e8d2 into master Oct 20, 2017
@gitlost gitlost deleted the patch-warning branch October 20, 2017 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gitlost gitlost gitlost approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

internal-api:wp-cli-file-cache scope:framework

Projects

None yet

Milestone

1.5.0

Development

Successfully merging this pull request may close these issues.

2 participants

@miya0001 @gitlost