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.

Sign up for GitHub

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

Jump to bottom

Support drivers containing gcs or s3 in driver name #3389

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Support drivers containing gcs or s3 in driver name #3389

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/MediaCollections/Filesystem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ public function getMediaDirectory(Media $media, ?string $type = null): string
? $media->conversions_disk
: $media->disk;

if (! in_array($diskDriverName, ['s3', 'gcs'], true)) {
if (! Str::contains($diskDriverName, ['s3', 'gcs'], true)) {
$this->filesystem->disk($diskName)->makeDirectory($directory);
Copy link
Contributor

Choose a reason for hiding this comment

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

We ran into an issue with this approach with detecting what actions to perform based on the driver.
Why? We are using Sentry and they create a wrapper for the storage drivers, see getsentry/sentry-laravel#775 for the details about the issue originally reported to Sentry.

The question is: Is this logic really needed at all? From my experience the Filestorage drivers handles the creation of a directory if needed. Meaning if you try to use a local storage driver and you try to write to the path, it will try to create the directory, and if you use S3, then it will just skip the step to make a directory.

cc @freekmurze @LukeAbell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea if the code is required. I would also assume you could just remove that line entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

@LukeAbell I created a PR for it here: #3397

}

Expand Down