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

If the underlying storage fails, emit an event #1450

Merged
merged 1 commit into from
May 18, 2024

Conversation

tarjei
Copy link
Contributor

@tarjei tarjei commented May 10, 2024

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


use Vich\UploaderBundle\Mapping\PropertyMapping;

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove this useless comment. Also, some style fixes are needed (see the actions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which action by the way ? I tried to find it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed now :) It seems to me that the PHPStan one is not related to this PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not yet.

By the way, I see some English errors in your text, please check it with some language tool (something like Grammarly)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. I focused on the markdown issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also tried to improve the language as per your input :)

try {
$this->storage->remove($obj, $mapping);
} catch (CannotWriteFileException $cannotWriteFileException) {
throw $cannotWriteFileException;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the point of throwing the exception and catching it below? Can't we just catch it directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exception is not cought today. Again, just trying to follow existing behaviour - but I'm quite fine with catching it.

src/Storage/StorageInterface.php Show resolved Hide resolved
@tarjei
Copy link
Contributor Author

tarjei commented May 10, 2024

@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?

if (!\file_exists($file)) {
throw new \Exception("File does not exist: ".$file);
}
if (!\unlink($file)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need separate exceptions here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, my rationale for doing so is that it provides a better debugging experience. I'll merge the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I plan to merge the commits when you're happy btw)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I didn't see that link no. I just applied the patch.

@garak
Copy link
Collaborator

garak commented May 17, 2024

PR is fine (already approved). Anyway, I meanwhile fixed the phpstan failure and would like to apply the check here.
Please rebase/merge master

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
Copy link
Contributor Author

tarjei commented May 18, 2024

@garak done! :) Oh the bliss of all green

@garak garak merged commit f6ed760 into dustin10:master May 18, 2024
10 checks passed
@garak
Copy link
Collaborator

garak commented May 18, 2024

Thank you!

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.

2 participants