Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:2.7fromxabbuh:issue-24097
Sep 7, 2017

Conversation

@xabbuh
Copy link
Member

QA
Branch?2.7
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#24097
LicenseMIT
Doc PR

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

@xabbuhxabbuh added this to the2.7 milestoneSep 5, 2017
@xabbuhxabbuh changed the base branch frommaster to2.7September 5, 2017 17:44
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.
thrownewIOException(sprintf('Unable to detect if the target directory %s exists.',$dir));
Copy link
Member

Choose a reason for hiding this comment

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

%s should be quoted with".

$oldCwd =getcwd();

if (!@chdir(dirname($dir))) {
// The target directory may exists, but we are unable to make a decision as we lack proper permissions
Copy link
Member

@javiereguiluzjaviereguiluzSep 6, 2017•
edited
Loading

Choose a reason for hiding this comment

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

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 executablebit even when the directory itself is present.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Better this way?

javiereguiluz reacted with thumbs up emoji
`is_dir()` returns `false` if the parent directory misses the executablebit even when the directory itself is present.
@fabpot
Copy link
Member

Thank you@xabbuh.

@fabpotfabpot merged commita0f9f2c intosymfony:2.7Sep 7, 2017
fabpot added a commit that referenced this pull requestSep 7, 2017
…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 executablebit even when the directory itself is present.Commits-------a0f9f2c check permissions if dump target dir is missing
@xabbuhxabbuh deleted the issue-24097 branchSeptember 7, 2017 16:54
@robfrawley
Copy link
Contributor

robfrawley commentedSep 8, 2017•
edited
Loading

@xabbuh@fabpot This completely breaksliip/imagine-bundle's user-facingWebPathResolver and a large number of tests that also rely onFilesystem::dumpFile(), for exampleResolveCacheTest.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 thisrecent Travis build. You can also clone the1.0 branch of the bundle and run the test suite. It succeeds on all version of Symfony except for2.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
Copy link
MemberAuthor

@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
Copy link
Contributor

robfrawley commentedSep 8, 2017•
edited
Loading

My issue is definitely thatsrc/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 onis_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 thechdir-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
Copy link
Contributor

@xabbuh@fabpot Can we please revert this PR in the immediate until another solution is found? AworkingFilesystem::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 release1.9.1 ofliip/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 Symfony2.7 from our Travis build matrix, even if temporarily. :-)

For now: please revert.

@fabpot
Copy link
Member

@robfrawley reverted

fabpot added a commit that referenced this pull requestSep 9, 2017
…is missing (xabbuh)"This reverts commitd74144f, reversingchanges made to2b79f48.
fabpot added a commit that referenced this pull requestSep 9, 2017
* 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
Copy link
Contributor

Wonderful; thanks for the quick response!

nicolas-grekas added a commit that referenced this pull requestSep 11, 2017
* 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
nicolas-grekas added a commit that referenced this pull requestSep 11, 2017
* 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
nicolas-grekas added a commit that referenced this pull requestSep 11, 2017
* 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
Copy link
MemberAuthor

@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?

@fabpotfabpot mentioned this pull requestSep 11, 2017
This was referencedOct 5, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

2.7

Development

Successfully merging this pull request may close these issues.

5 participants

@xabbuh@fabpot@robfrawley@javiereguiluz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp