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

Give the current App instance to FilesystemManager #458

Merged

Conversation

AlexVanderbist
Copy link
Contributor

FilesystemManager and its custom/extended drivers rely on the $filesystemManager->app instance to provide the app config. If the config changes at runtime (e.g. multi-tenancy environment or via a package service provider) the FilesystemManager keeps using the old application instance and the wrong config values.

This PR uses laravel/framework#40058 to fix this issue. A test is included and the laravel/framework version is bumped to make sure the FilesystemManager::setApplication() method from the aforementioned PR is always available.

@taylorotwell taylorotwell merged commit 2eba7a8 into laravel:1.x Jan 6, 2022
@driesvints
Copy link
Member

@AlexVanderbist this PR broke our build. Can you send in a fix? Otherwise it might be best we revert this for now.

@AlexVanderbist
Copy link
Contributor Author

Looking at it right now! Saw the failing test and didn't think it would get merged 😅

@driesvints
Copy link
Member

@AlexVanderbist it's always best to place your PR in draft when it's not finished yet. I know the build indicates it's not passing but sometimes, like now, we can miss that.

@AlexVanderbist
Copy link
Contributor Author

@driesvints that's my bad! Wasn't failing locally so didn't realise until it was too late. Got a fix ready in #459 tho :)

@driesvints
Copy link
Member

@AlexVanderbist Thanks! :)

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