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

[5.1] Do not allow empty cache paths #14291

Merged
merged 1 commit into from
Jul 18, 2016
Merged

[5.1] Do not allow empty cache paths #14291

merged 1 commit into from
Jul 18, 2016

Conversation

GrahamCampbell
Copy link
Member

Replaces #14277. Closes #14271.

*/
public function __construct(Filesystem $files, $cachePath)
{
if (! $cachePath) {
throw new InvalidArgumentException('The cache path must be non-empty.');
Copy link
Contributor

Choose a reason for hiding this comment

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

"non-empty" might be ambiguous in this context.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's ok. false and null are "empty" in php.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant because it's referring to a path which might be empty (not contain any files).

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, one of these may be better:

  • The cache path must be a non-empty value.
  • The cache path must not be an empty value.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's been changed in d9ed009

@taylorotwell taylorotwell merged commit 90f2edd into 5.1 Jul 18, 2016
@GrahamCampbell GrahamCampbell deleted the compiler branch July 18, 2016 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants