FileSystem driver now tries to create cache folder before throwing InvalidArgumentException #66

Merged
merged 1 commit into from Mar 30, 2013

Conversation

Projects
None yet
4 participants
Contributor

lolautruche commented Mar 8, 2013

Hello

Fixes tedivm/TedivmStashBundle#15

This is a very small change allowing FileSystem driver to try to create the cache folder if it doesn't already exist. If folder creation fails, then the exception is thrown.

Note the usage of the silent operator (@). It's here only to avoid a verbose warning if folder creation fails since we want to throw an exception.

@Baachi Baachi and 1 other commented on an outdated diff Mar 8, 2013

src/Stash/Driver/FileSystem.php
@@ -385,7 +385,10 @@ protected function checkFileSystemPermissions()
}
if(!is_dir($this->cachePath)) {
@Baachi

Baachi Mar 8, 2013

Contributor

Maybe this is better:

if (!is_dir($this->cachepath) && !mkdir($this->cachePath, $this->dirPermissions, true)) {
    throw new InvalidArgumentException(sprintf('Failed to create cache path %s', $this->cachePath));
}
@lolautruche

lolautruche Mar 8, 2013

Contributor

You're right 😃 . Fixed

andrerom referenced this pull request Mar 8, 2013

Closed

New stable? (0.10.3) #67

Contributor

lolautruche commented Mar 11, 2013

Note : Travis is not failing because of this patch, but because of Memached extension installation (probably because of this : http://about.travis-ci.org/blog/2013-03-08-preinstalled-php-extensions/).

Owner

tedivm commented Mar 11, 2013

Yup, I actually opened the tickets with Travis about it-

travis-ci/travis-ci#970
travis-ci/travis-ci#971

I've also created a new branch to resolve the testing issues-

https://github.com/tedivm/Stash/tree/travis_test

Once it's resolved I'll merge the travis_test branch back into mainline, and then I'll ask you to just update your PR with those tests so we can see that things are running smoothly.

Contributor

lolautruche commented Mar 11, 2013

Ok, @tedivm . Just let me know 😃

Contributor

andrerom commented Mar 18, 2013

@lolautruche Travis should be fixed now, so you can do a git ci --amend & and a force push to get it to test this.

Contributor

lolautruche commented Mar 18, 2013

@tedivm It seems that your tickets with Travis and Memcached are fixed now, and your travis_test branch looks it passes tests. Could you then merge your travis_test branch into master so I can rebase ?
Thanks !

Owner

tedivm commented Mar 18, 2013

The travis_test branch isn't actually passing them all- it's skipping the APC tests. I'm trying to get that resolved (I think I know why it's occurring) and merge that back into mainline. I"ll update here once that's handled.

Contributor

lolautruche commented Mar 25, 2013

@tedivm Sorry to insist... Actually we really need this in for the next version of eZ Publish and we'll soon enter in a deep code freeze mode. This PR is not related to APC or Memcached since it's only about the FileSystem driver.
If you really want to wait your issue to be fixed, OK but I'll have to point eZ Publish on my fork which I don't really want :-/.

Thanks.

lolautruche referenced this pull request in ezsystems/ezpublish-kernel Mar 26, 2013

Merged

Made Stash FileSystem driver the default one #274

@tedivm tedivm added a commit that referenced this pull request Mar 30, 2013

@tedivm tedivm Merge pull request #66 from lolautruche/filesystem_dir_creation
FileSystem driver now tries to create cache folder before throwing InvalidArgumentException
1606a2b

@tedivm tedivm merged commit 1606a2b into tedious:master Mar 30, 2013

1 check failed

default The Travis build could not complete due to an error
Details

@pborreli pborreli pushed a commit to pborreli/ezpublish-kernel that referenced this pull request Nov 19, 2013

@lolautruche lolautruche Updated Stash in composer.json to use a fork, waiting for filesystem …
…driver fix is pulled in tedivm/Stash.

Ref tedious/Stash#66
01ef46e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment