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

[Filesystem] Fix dumpFile stat failed error hitting custom handler #54878

Merged
merged 1 commit into from May 16, 2024

Conversation

acoulton
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues See below
License MIT

Since #54471, dumpFile will trigger a fileperms(): stat failed error when writing to a filename that does not yet exist. This was silenced from PHP's default handler with the @ operator.

However, the error is still passed to any custom handler that the application has registered, and can therefore cause exceptions or spurious logging depending on the implementation of the handler.

The better solution, which is consistent with all other calls to native functions in this class, would be to use self::box to catch and ignore the potential error so that it never leaks outside this class.

Since symfony#54471, dumpFile will trigger a `fileperms(): stat failed`
error when writing to a filename that does not yet exist. This
was silenced from PHP's default handler with the `@` operator.

However, the error is still passed to any custom handler that the
application has registered, and can therefore cause exceptions or
spurious logging depending on the implementation of the handler.

The better solution, which is consistent with all other calls to
native functions in this class, would be to use `self::box` to
catch and ignore the potential error so that it never leaks
outside this class.
@acoulton
Copy link
Contributor Author

As far as I can see the coding standards and test failures are unrelated to the change in this branch - but please let me know if you want any changes.

@xabbuh
Copy link
Member

xabbuh commented May 10, 2024

The proposed solution works for me but:

However, the error is still passed to any custom handler that the application has registered, and can therefore cause exceptions or spurious logging depending on the implementation of the handler.

This is still an error in the custom error handler if it doesn't respect the silencing of errors and should be fixed nonetheless as an error handler throwing no matter if the error was silenced can still throw too eagerly.

@xabbuh xabbuh added this to the 5.4 milestone May 10, 2024
@acoulton
Copy link
Contributor Author

This is still an error in the custom error handler if it doesn't respect the silencing of errors and should be fixed nonetheless as an error handler throwing no matter if the error was silenced can still throw too eagerly.

That's probably true. At the same time, I don't think a library should be relying on the silencing operator to suppress errors that are emitted during expected usage of a method - in particular if it hasn't done so in the past, since this might break existing applications where the over-eager throw has not historically caused any problems.

@carsonbot carsonbot changed the title [Filesystem] Fix dumpFile stat failed error hitting custom handler Fix dumpFile stat failed error hitting custom handler May 13, 2024
@carsonbot carsonbot changed the title Fix dumpFile stat failed error hitting custom handler [Filesystem] Fix dumpFile stat failed error hitting custom handler May 15, 2024
@nicolas-grekas
Copy link
Member

Thank you @acoulton.

@nicolas-grekas nicolas-grekas merged commit f68a8df into symfony:5.4 May 16, 2024
6 of 12 checks passed
@acoulton acoulton deleted the fs-fileperms-error-handler branch May 16, 2024 17:32
@fabpot fabpot mentioned this pull request May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants