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

Support drivers containing gcs or s3 in driver name #3389

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

LukeAbell
Copy link
Contributor

We have multiple gcs drivers (Ex. gcs-private, gcs-public).

This PR detects GCS as long as it's in the driver name.

@sebastiandedeyne sebastiandedeyne merged commit 87fb770 into spatie:main Sep 26, 2023
11 checks passed
@@ -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

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.

3 participants