If the underlying storage fails, emit an event #1450
Conversation
tarjei
force-pushed
the
I-1449-exceptionhandling
branch
from
e37406f
to
55bce43
Compare
src/Event/ErrorEvent.php
Outdated
|
|
||
| use Vich\UploaderBundle\Mapping\PropertyMapping; | ||
|
|
||
| /** |
There was a problem hiding this comment.
please remove this useless comment. Also, some style fixes are needed (see the actions)
There was a problem hiding this comment.
Which action by the way ? I tried to find it.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks! Fixed now :) It seems to me that the PHPStan one is not related to this PR
There was a problem hiding this comment.
Not yet.
By the way, I see some English errors in your text, please check it with some language tool (something like Grammarly)
There was a problem hiding this comment.
My bad. I focused on the markdown issues.
There was a problem hiding this comment.
I also tried to improve the language as per your input :)
src/Handler/UploadHandler.php
Outdated
| try { | ||
| $this->storage->remove($obj, $mapping); | ||
| } catch (CannotWriteFileException $cannotWriteFileException) { | ||
| throw $cannotWriteFileException; |
There was a problem hiding this comment.
what's the point of throwing the exception and catching it below? Can't we just catch it directly?
There was a problem hiding this comment.
This exception is not cought today. Again, just trying to follow existing behaviour - but I'm quite fine with catching it.
|
@garak thanks for a very quick review! I'm still a bit unsure if an event is the correct approach. Maybe we could have a config option to either throw on failure or swallow it? |
tarjei
force-pushed
the
I-1449-exceptionhandling
branch
from
312ec61
to
abce1e5
Compare
src/Storage/FileSystemStorage.php
Outdated
| if (!\file_exists($file)) { | ||
| throw new \Exception("File does not exist: ".$file); | ||
| } | ||
| if (!\unlink($file)) { |
There was a problem hiding this comment.
We don't need separate exceptions here
There was a problem hiding this comment.
OK, my rationale for doing so is that it provides a better debugging experience. I'll merge the two.
There was a problem hiding this comment.
(I plan to merge the commits when you're happy btw)
There was a problem hiding this comment.
This is linked below, in case you missed it: https://github.styleci.io/analyses/DynDKg?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
There was a problem hiding this comment.
Thanks! I didn't see that link no. I just applied the patch.
tarjei
force-pushed
the
I-1449-exceptionhandling
branch
from
43625fd
to
4246b95
Compare
|
PR is fine (already approved). Anyway, I meanwhile fixed the phpstan failure and would like to apply the check here. |
If the underlying storage fails, we emit an event that can be handled as the user wants. The behaviour is to throw exceptions on write but not on remove as this seems to be the semantics as they are in the library prior to this change.
tarjei
force-pushed
the
I-1449-exceptionhandling
branch
from
4246b95
to
add4e18
Compare
|
@garak done! :) Oh the bliss of all green |
|
Thank you! |
If the underlying storage fails, we emit an event that can be handled as the user wants.
The behaviour is to throw exceptions on write but not on remove as this seems to be the semantics as they are in the library prior to this change.
It seems to me that the error behavior of remove operations is a bit uneven. If a write fails in the native filesystem it seems to me that that will trigger some kind of error.
Anyhow, I think this keeps BC but introduces an event.
Ref #1449