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

Remove unnecessary directory creation #3397

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

grEvenX
Copy link
Contributor

@grEvenX grEvenX commented Sep 28, 2023

This PR suggests to remove the conditional directory creation logic in the getMediaLibrary() function.
Unless it's a very specific reason to do it, I think it would be better to avoid checking for a specific driver to run the logic for creating a directory or not (since that can cause unexpected issues).
In our case we ran into an issue with the solution since another library was masking the real driver name (so it wasn't called anything with "s3" anymore), and since our S3 permission setup only allowed writing files, not creating an empty directory, the storing of the file caused an exception to be thrown.
See relevant comment at https://github.com/spatie/laravel-medialibrary/pull/3389/files#r1338270714.

According to Flysystem, the actual driver creates the required directory when attempting to write a file;

When writing files, the directory you’re writing to will be created automatically if and when that is required in the filesystem you’re writing to. If your filesystem does not require directories to exist (like AWS S3), the directory is not created. This is a performance consideration. Of course, you can always create the directory yourself by using the createDirectory operation.

This commit removes the directory creation for the 'getMediaLibrary' function.
According to Flysystem, the actual driver creates the required directory when attempting
to write a file.

> When writing files, the directory you’re writing to will be created automatically if and when that is required in the filesystem you’re writing to. If your filesystem does not require directories to exist (like AWS S3), the directory is not created. This is a performance consideration. Of course, you can always create the directory yourself by using the createDirectory operation.
@freekmurze
Copy link
Member

How have you tested that the package works without this logic?

@grEvenX
Copy link
Contributor Author

grEvenX commented Oct 13, 2023

@freekmurze I have tested with Local Storage and S3 storage on where I have used media conversions, but haven't tested all drivers Flysystem works with and I can't guarantee that it works on all drivers. If this would cause an issue or an error I believe a bug-report should be filed to Flysystem as they have documented this behaviour.

I've also reviewed all code calling the changed method and verified that all operations related to the directory are done through Flysystem and verified that the current test suite passes all tests after this change.

@freekmurze freekmurze merged commit ae3107b into spatie:main Oct 24, 2023
11 checks passed
@freekmurze
Copy link
Member

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