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

Throw error if disk cannot be accessed #3067

Merged
merged 7 commits into from
Nov 27, 2022
Merged

Throw error if disk cannot be accessed #3067

merged 7 commits into from
Nov 27, 2022

Conversation

Silver343
Copy link
Contributor

This PR changes the behaviour when a filesytem cannot be accessed as seen in #3010, it will throw and exception and not create a new media record.

@patinthehat
Copy link
Collaborator

@Silver343 Maybe a better approach is to perform a check to see if the disk can be accessed first, and immediately fail if necessary instead of using a transaction. Thoughts?

@patinthehat
Copy link
Collaborator

@Silver343 I've refactored your code to remove the use of database transactions. Are you okay with the changes I made?

ping @freekmurze

@Silver343
Copy link
Contributor Author

looks good to me, not sure if the change in return type to add and add remote would be considered breaking changes.

@patinthehat
Copy link
Collaborator

@freekmurze I believe this PR is now ready for review.

tests/Feature/FileAdder/IntegrationTest.php Outdated Show resolved Hide resolved
tests/Feature/FileAdder/IntegrationTest.php Outdated Show resolved Hide resolved
@patinthehat
Copy link
Collaborator

@freekmurze To keep this PR moving along and get it merged/closed ASAP, I've pushed the changes you requested to the unit test on behalf of the original PR author (since they were minor).

Would you please re-review at your convenience? Thanks! 👍

@freekmurze freekmurze merged commit 4acff99 into spatie:main Nov 27, 2022
@freekmurze
Copy link
Member

Thanks!

@Silver343 Silver343 deleted the Throw-error-is-disk-cannot-be-accessed branch November 27, 2022 12:27
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.

3 participants