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

[11.x] Fix issue where $name variable in non base config file becomes it's key #52738

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

rojtjo
Copy link
Contributor

@rojtjo rojtjo commented Sep 11, 2024

I noticed a bug in one of my apps where it seemed like my published config file was not loaded. After debugging I noticed that
the config file was actually loaded, but under a different key. It looks like this happens when you define a variable named $name in one of your non base config files.

This happens because after requiring the config file, the $name variable is used to call $repository->set($name, $config);.

I fixed this issue by wrapping the require statement in a closure which makes it run in isolation.

@taylorotwell
Copy link
Member

Can you give recreation instructions?

@taylorotwell taylorotwell marked this pull request as draft September 11, 2024 20:16
@rojtjo
Copy link
Contributor Author

rojtjo commented Sep 11, 2024

I've prepared an example at rojtjo/laravel-52738. In the example I've created a config file named foo.php which defines the variable $name at the top and I've also included a failing test.

The issue happens because we inherit all variables from the loadConfigurationFile method. This means that defining any of the variables used in that method, $name in my example, actually redefines them instead.

@rojtjo rojtjo marked this pull request as ready for review September 13, 2024 13:49
@taylorotwell taylorotwell merged commit da28aab into laravel:11.x Sep 13, 2024
30 of 31 checks passed
@taylorotwell
Copy link
Member

lol - that's kinda wild.

@rojtjo
Copy link
Contributor Author

rojtjo commented Sep 14, 2024

Tell me about it 😅

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