[Filesystem] check permissions if dump target dir is missing #24105

xabbuh · 2017-09-05T17:44:05Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#24097
License	MIT
Doc PR

is_dir() returns false if the parent directory misses the executable
bit even when the directory itself is present.

fabpot · 2017-09-05T18:49:56Z

src/Symfony/Component/Filesystem/Filesystem.php

+            if (!@chdir(dirname($dir))) {
+                // The target directory may exists, but we are unable to make a decision as we lack proper permissions
+                // to enter its parent directory.
+                throw new IOException(sprintf('Unable to detect if the target directory %s exists.', $dir));


%s should be quoted with ".

javiereguiluz · 2017-09-06T13:35:52Z

src/Symfony/Component/Filesystem/Filesystem.php

+            $oldCwd = getcwd();
+
+            if (!@chdir(dirname($dir))) {
+                // The target directory may exists, but we are unable to make a decision as we lack proper permissions


may exists -> may exist

And maybe you could add this important information in the PHP comment (you only added in the PR description):

is_dir() returns false if the parent directory misses the executable bit even when the directory itself is present.

Better this way?

`is_dir()` returns `false` if the parent directory misses the executable bit even when the directory itself is present.

fabpot · 2017-09-07T14:43:28Z

Thank you @xabbuh.

…ng (xabbuh) This PR was merged into the 2.7 branch. Discussion ---------- [Filesystem] check permissions if dump target dir is missing | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24097 | License | MIT | Doc PR | `is_dir()` returns `false` if the parent directory misses the executable bit even when the directory itself is present. Commits ------- a0f9f2c check permissions if dump target dir is missing

robfrawley · 2017-09-08T15:09:59Z

@xabbuh @fabpot This completely breaks liip/imagine-bundle's user-facing WebPathResolver and a large number of tests that also rely on Filesystem::dumpFile(), for example ResolveCacheTest.php.

I don't believe we're using the method incorrectly; we're passing the method a path (some of which point to paths that already exists and others that don't, sometimes with multiple levels of not-yet-created directories) along with its contents. For test failures, see this recent Travis build. You can also clone the 1.0 branch of the bundle and run the test suite. It succeeds on all version of Symfony except for 2.7 (which is the only one with this patch).

Are we doing something wrong? Or did this change break things it should not have? I'll take a closer look shortly...

xabbuh · 2017-09-08T15:24:19Z

@robfrawley Thanks for stepping up here. I think you are right and this change was not completely right. I'll check if we can fix it easily. Otherwise I suggest we revert this PR as the improved error reporting is not worth it if it breaks functionality.

robfrawley · 2017-09-08T15:58:19Z

My issue is definitely that src/Symfony/Component/Filesystem/Filesystem.php tries to enter a directory that might not exist. Something like the following fixes our usage (it works back from the provided directory path and finds the first one that exists). The problem with it is it likely nullifies the point of the new check, as it again relies on is_dir() but I just wanted to confirm the cause:

535a536
>             $exists = $dir;
537c538,542
<             if (!@chdir(dirname($exists))) {
---
>             do {
>                 $exists = dirname($exists);
>             } while (!empty($exists) && !is_dir($exists));
> 
>             if (!empty($exists) && !@chdir(dirname($exists))) {

A straight removal of the chdir-related code resolves the issues as well, of course:

535,544d534
<             $oldCwd = getcwd();
< 
<             if (!@chdir(dirname($exists))) {
<                 // When the parent directory misses the executable permission bit, we are unable to enter it and thus
<                 // cannot check if the target directory exists.
<                 throw new IOException(sprintf('Unable to detect if the target directory "%s" exists.', $dir));
<             }
< 
<             chdir($oldCwd);
<

robfrawley · 2017-09-09T00:57:37Z

@xabbuh @fabpot Can we please revert this PR in the immediate until another solution is found? A working Filesystem::dumpFile() method is much more important than the goal of this PR (better error handling), and such can be followed up later in a subsequent PR.

The bugfix release 1.9.1 of liip/imagine-bundle needs to be released ASAP, but (as this breaks our test suite badly) I don't feel great about tagging a release the is associated with a failing Travis build (nor would I like for our main README's build badge to show failing once the release is tagged). I'd also prefer not to remove Symfony 2.7 from our Travis build matrix, even if temporarily. :-)

For now: please revert.

fabpot · 2017-09-09T01:12:23Z

@robfrawley reverted

…is missing (xabbuh)" This reverts commit d74144f, reversing changes made to 2b79f48.

* 2.7: Revert "bug #24105 [Filesystem] check permissions if dump target dir is missing (xabbuh)" [Filesystem] skip tests if not applicable [Fabbot] Do not run php-cs-fixer if there are no change in src/ [Security] Fix exception when use_referer option is true and referer is not set or empty Get KERNEL_DIR through $_ENV too for KernelTestCase check permissions if dump target dir is missing

robfrawley · 2017-09-09T02:26:09Z

Wonderful; thanks for the quick response!

* 2.8: Revert "bug #24105 [Filesystem] check permissions if dump target dir is missing (xabbuh)" [Filesystem] skip tests if not applicable [Fabbot] Do not run php-cs-fixer if there are no change in src/ [Security] Fix exception when use_referer option is true and referer is not set or empty Get KERNEL_DIR through $_ENV too for KernelTestCase check permissions if dump target dir is missing

* 3.3: Revert "bug #24105 [Filesystem] check permissions if dump target dir is missing (xabbuh)" [Filesystem] skip tests if not applicable [Fabbot] Do not run php-cs-fixer if there are no change in src/ [Security] Fix exception when use_referer option is true and referer is not set or empty [HttpKernel] "controller.service_arguments" services should be public Get KERNEL_DIR through $_ENV too for KernelTestCase Get KERNEL_CLASS through $_ENV too check permissions if dump target dir is missing

* 3.4: [SecurityBundle] Fix valid provider considered undefined Revert "bug #24105 [Filesystem] check permissions if dump target dir is missing (xabbuh)" [Filesystem] skip tests if not applicable [Fabbot] Do not run php-cs-fixer if there are no change in src/ [ExpressionLanguage] make a proposal in SyntaxError message [Security] Fix exception when use_referer option is true and referer is not set or empty [HttpKernel] "controller.service_arguments" services should be public Get KERNEL_DIR through $_ENV too for KernelTestCase Get KERNEL_CLASS through $_ENV too check permissions if dump target dir is missing

xabbuh · 2017-09-11T09:07:59Z

@fabpot thanks 👍

@robfrawley I have some code locally that would probably fix the issue that you experienced. Though I don't think it's worth it as the error we tried to detect here is an extreme edge case IMO. However, we have #24134 which may provide some other improved error handling. Would you mind testing that PR with your code base?

xabbuh added this to the 2.7 milestone Sep 5, 2017

carsonbot added Status: Needs Review Filesystem Bug labels Sep 5, 2017

xabbuh mentioned this pull request Sep 5, 2017

Create symlink target directory if needed #24097

Closed

xabbuh changed the base branch from master to 2.7 September 5, 2017 17:44

fabpot reviewed Sep 5, 2017

View reviewed changes

xabbuh force-pushed the issue-24097 branch from a7fc958 to 64b390a Compare September 5, 2017 19:23

javiereguiluz reviewed Sep 6, 2017

View reviewed changes

check permissions if dump target dir is missing

a0f9f2c

`is_dir()` returns `false` if the parent directory misses the executable bit even when the directory itself is present.

xabbuh force-pushed the issue-24097 branch from 64b390a to a0f9f2c Compare September 6, 2017 16:35

javiereguiluz approved these changes Sep 6, 2017

View reviewed changes

fabpot merged commit a0f9f2c into symfony:2.7 Sep 7, 2017

xabbuh deleted the issue-24097 branch September 7, 2017 16:54

robfrawley mentioned this pull request Sep 8, 2017

[Command] New option shortcut collides with Symfony pre-3.x default option shortcuts, causing exception when running cache:resolve command liip/LiipImagineBundle#988

Closed

robfrawley mentioned this pull request Sep 8, 2017

[Console] Renamed resolve command's -s option to -m (-s conflicts with core Symfony 2.x cmd option), machine readable added to remove command, and both commands output aligned liip/LiipImagineBundle#991

Merged

fabpot added a commit that referenced this pull request Sep 9, 2017

Revert "bug #24105 [Filesystem] check permissions if dump target dir …

7793b79

…is missing (xabbuh)" This reverts commit d74144f, reversing changes made to 2b79f48.

robfrawley mentioned this pull request Sep 11, 2017

[Filesystem] improved errors for invalid dump targets #24134

Closed

fabpot mentioned this pull request Sep 11, 2017

Release v3.3.9 #24156

Merged

This was referenced Oct 5, 2017

Release v2.7.35 #24450

Merged

Release v2.8.28 #24451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Filesystem] check permissions if dump target dir is missing #24105

[Filesystem] check permissions if dump target dir is missing #24105

xabbuh commented Sep 5, 2017

fabpot Sep 5, 2017

javiereguiluz Sep 6, 2017 •

edited

Loading

xabbuh Sep 6, 2017

fabpot commented Sep 7, 2017

robfrawley commented Sep 8, 2017 •

edited

Loading

xabbuh commented Sep 8, 2017

robfrawley commented Sep 8, 2017 •

edited

Loading

robfrawley commented Sep 9, 2017

fabpot commented Sep 9, 2017

robfrawley commented Sep 9, 2017

xabbuh commented Sep 11, 2017

[Filesystem] check permissions if dump target dir is missing #24105

[Filesystem] check permissions if dump target dir is missing #24105

Conversation

xabbuh commented Sep 5, 2017

fabpot Sep 5, 2017

Choose a reason for hiding this comment

javiereguiluz Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

xabbuh Sep 6, 2017

Choose a reason for hiding this comment

fabpot commented Sep 7, 2017

robfrawley commented Sep 8, 2017 • edited Loading

xabbuh commented Sep 8, 2017

robfrawley commented Sep 8, 2017 • edited Loading

robfrawley commented Sep 9, 2017

fabpot commented Sep 9, 2017

robfrawley commented Sep 9, 2017

xabbuh commented Sep 11, 2017

javiereguiluz Sep 6, 2017 •

edited

Loading

robfrawley commented Sep 8, 2017 •

edited

Loading

robfrawley commented Sep 8, 2017 •

edited

Loading