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

fix: Avoid manipulating config when resolving disks #901

Merged
merged 4 commits into from
Jun 5, 2024

Conversation

hansnn
Copy link
Contributor

@hansnn hansnn commented May 30, 2024

This commit updates the storage integration such that it does not manipulate the filesystems config when a disk is resolved.
Doing so fixes an issue when using scoped disks where the disk's prefix property is injected into the parent disk's config, making the parent disk unusable from that point forward.

The added test doesn't replicate the issue with scoped disks specifically because the league/flysystem-path-prefixing package is required to use scoped disks. The best I could do was to assert that the config isn't being manipulated when a disk is resolved, which seems sensible in and of itself.

Tagging you here, @spawnia, as you may have some insight into why the "hack" this commit removes was added in the first place, and if it's still needed (ref #726).

This commit updates the storage integration such that it does not manipulate the filesystems
config when a disk is resolved.
This fixes an issue when using scoped disks where the disk's prefix property was injected
into the parent disk's config, making the parent disk unusable from that point forward.
@hansnn
Copy link
Contributor Author

hansnn commented May 30, 2024

Seeing that the CI failed I now understand why the hack was added in the first place: The $config parameter is not available in FilesystemManager::resolve in Laravel < 8 (it was probably added in some version 8.X.X). I guess this fix can't be applied as-is, but I'll leave the PR open for now while investigating if there is a workaround to make it compatible with Laravel < 8 (unless you think it should be closed).

@stayallive
Copy link
Collaborator

stayallive commented May 30, 2024

It would be nice if scoped disks will work correctly of course but we should also make sure that it works for all Laravel versions of course. So I'm happy to see if you can find a workable workaround to make this work for all supported versions.

Edit: I marked it as draft for now, feel free to mark as ready whenever of course 👍

@stayallive stayallive marked this pull request as draft May 30, 2024 08:19
@hansnn
Copy link
Contributor Author

hansnn commented May 30, 2024

The only solutions I can think of are to copy the resolve method from the Laravel library and use that directly instead of calling $this->resolve(), or to use reflection to check if the method accepts the $config parameter.

I'm not sure if there is any willingness to include either of those options. What do you think, @stayallive?

@hansnn hansnn marked this pull request as ready for review May 31, 2024 06:44
@hansnn
Copy link
Contributor Author

hansnn commented May 31, 2024

Pushed a commit that should work with all Laravel versions without using reflection or copying code from the Laravel repository. Kudos to @grEvenX who came up with the solution.

Copy link
Contributor

@spawnia spawnia left a comment

Choose a reason for hiding this comment

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

Looks like a sensible change to me.

Copy link
Collaborator

@stayallive stayallive left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to make the PR 👍

@stayallive stayallive merged commit 3adfd3c into getsentry:master Jun 5, 2024
22 checks passed
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.

None yet

3 participants