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

[9.x] Report file system exceptions #41279

Closed
wants to merge 1 commit into from
Closed

[9.x] Report file system exceptions #41279

wants to merge 1 commit into from

Conversation

ankurk91
Copy link
Contributor

@ankurk91 ankurk91 commented Mar 1, 2022

@taylorotwell

This is an attempt to fix issue #41269

Suppressing all those file system exceptions was a bad decision made in PR #33612

  • There is no breaking change, all methods still return boolean
  • We are just reporting the exception to respective logger

Imaging your server's hard disk runs out of space or AWS S3 service went down, you will never gets notified about it and your customers will keep complaining about their missing uploaded files. I have spent hours already to figure out why my S3 uploads are not working.

This PR reports those errors so at-least the developer can look into logs and see what went wrong.

Willing to see feedback form the community on this solution. :)

Copy link
Contributor

@Jubeki Jubeki left a comment

Choose a reason for hiding this comment

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

You forgot lots of Semicolons.

I also think it is a good idea to at least report the errors.

@driesvints
Copy link
Member

We cannot use a global helper in this class. You'll need to set the logger onto the class with a setter method and add a new config option that by default disables it. Then people can opt-into this. Please resubmit with these changes and make sure tests are passing. Thanks

@driesvints driesvints closed this Mar 1, 2022
@ankurk91
Copy link
Contributor Author

ankurk91 commented Mar 1, 2022

driesvints
Thanks, i will re-attempt it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants