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] Storage::put() no longer throws an exception #41269

Closed
ankurk91 opened this issue Feb 28, 2022 · 8 comments
Closed

[9.x] Storage::put() no longer throws an exception #41269

ankurk91 opened this issue Feb 28, 2022 · 8 comments

Comments

@ankurk91
Copy link
Contributor

  • Laravel Version: 9.x
  • PHP Version: Does not matter
  • Database Driver & Version: Does not matter

Description:

Storage::put() and similar methods no longer throws and and silently return false, the actual error is never logged or visible to developer

Here is responsible code

try {
if ($contents instanceof StreamInterface) {
$this->driver->writeStream($path, $contents->detach(), $options);
return true;
}
is_resource($contents)
? $this->driver->writeStream($path, $contents, $options)
: $this->driver->write($path, $contents, $options);
} catch (UnableToWriteFile $e) {
return false;
}
return true;

PR #33612

Steps To Reproduce:

Update your .env with
FILESYSTEM_DISK=s3 then make a mistake in your AWS credentials or shutdown your minio server

Try to upload a file via Storage::put("test.text","Some text");

Notice that no exception will be thrown, nothing can be found in storage/logs either.
It is impossible to get underlying adaptor error, (in my case S3)

What can be done next

Throw the exception like before (v8.x) and let end user handle it. OR
Document this change in upgrade guide.

@driesvints
Copy link
Member

Flysystem v2 & v3 now throw UnableToWriteFile so there's no way anymore to properly indicate why a write operation didn't succeed. For Laravel 9 we decided to consolidate all methods in the FilesystemAdapter to only work with boolean returns and catch all internal Flysystem exception throwing.

I've sent in a PR to the upgrade guide: laravel/docs#7755

We are however, maybe considering to implement an optional logger to log these exceptions instead.

@ankurk91
Copy link
Contributor Author

Suppressing such exceptions is a bad idea.
Checking for boolean all over the project, whether uploading to S3 (cloud) failed or not, is no where developer friendly.
Laravel is known for developer happiness but no more. :(

Since this behaviour was not documented in upgrade guide, it should be easy to bring back old behaviour, or at-least throw a custom exception to halt the execution.

@driesvints
Copy link
Member

Suppressing such exceptions is a bad idea.

I personally too, more prefer exception throwing. However, we decided to pursue boolean returns instead to consolidate the workings of the filesystem.

it should be easy to bring back old behaviour

We cannot do that as Laravel v9 is now a month old and doing this will introduce breaking changes for apps that already upgraded.

@mike-code

This comment was marked as abuse.

@taylorotwell
Copy link
Member

Hey all! We have an idea for improving this for you - going to look into it.

@jjkavalam
Copy link

I am a newbie to Laravel, but experienced in other languages/frameworks. Quite impressed with Laravel's developer friendliness overall.

However, when trying to integrate with cloud storage, i did end up in a situation where there was no way to know what went wrong. I happened to see 'throw' => false by chance in config/filesystems.php and happily flipped it to true !

Could you please mention the existence of throw to the primary documentation page ?

Really appreciate all the hard work !
@driesvints @taylorotwell

@driesvints
Copy link
Member

We can always review a PR if you can send one 👍

@jjkavalam
Copy link

@driesvints Sure, i have opened PR: laravel/docs#8961

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

No branches or pull requests

5 participants