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

[HttpFoundation] Check file exists before unlink #29764

Merged
merged 1 commit into from Jan 25, 2019

Conversation

Projects
None yet
6 participants
@adam-mospan
Copy link
Contributor

adam-mospan commented Jan 3, 2019

Check file exists to prevent ErrorException

Q A
Branch? 2.6
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR symfony/symfony-docs#...
@@ -299,8 +299,7 @@ public function sendContent()
fclose($out);
fclose($file);
if ($this->deleteFileAfterSend) {
if ($this->deleteFileAfterSend && file_exists($this->file->getPathname())) {

This comment has been minimized.

@ro0NL

ro0NL Jan 4, 2019

Contributor

doenst fopen already fail in this case?

This comment has been minimized.

@adam-mospan

adam-mospan Jan 4, 2019

Author Contributor

The file can be deleted after fopen() while it is being sent to the user.

This comment has been minimized.

@ro0NL

ro0NL Jan 4, 2019

Contributor

got ya :)

This comment has been minimized.

@xabbuh

xabbuh Jan 4, 2019

Member

But the file could also be removed between the calls to file_exists() and unlink(). Maybe it would be better to silence the delete operation instead.

This comment has been minimized.

@adam-mospan

adam-mospan Jan 4, 2019

Author Contributor

Downloading is a much longer process than file_exists()

This comment has been minimized.

@adam-mospan

adam-mospan Jan 13, 2019

Author Contributor

Also, I do not think that @unlink($this->file->getPathname()); would be a good solution here, it will ignore all errors, and not just 'No such file or directory'.
What else needs to be done for this to be merged ?

@chalasr chalasr added this to the 3.4 milestone Jan 4, 2019

@nicolas-grekas nicolas-grekas changed the title Check file exists before unlink [HttpFoundation] Check file exists before unlink Jan 25, 2019

@nicolas-grekas nicolas-grekas changed the base branch from master to 3.4 Jan 25, 2019

@nicolas-grekas nicolas-grekas force-pushed the adam-mospan:patch-1 branch from 8e6b227 to 1954187 Jan 25, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 25, 2019

Thank you @adam-mospan.

@nicolas-grekas nicolas-grekas merged commit 1954187 into symfony:3.4 Jan 25, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jan 25, 2019

bug #29764 [HttpFoundation] Check file exists before unlink (adam-mos…
…pan)

This PR was submitted for the master branch but it was squashed and merged into the 3.4 branch instead (closes #29764).

Discussion
----------

[HttpFoundation] Check file exists before unlink

Check file exists to prevent ErrorException

| Q             | A
| ------------- | ---
| Branch?       | 2.6 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

1954187 [HttpFoundation] Check file exists before unlink

This was referenced Jan 29, 2019

@adam-mospan adam-mospan deleted the adam-mospan:patch-1 branch Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment