-
Notifications
You must be signed in to change notification settings - Fork 170
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
Set application key in .env file (if exists) or fallback config file. #560
Set application key in .env file (if exists) or fallback config file. #560
Conversation
@ericp-mrel Tested and it works fine, but there's one more scenario we need to cover here. A not-so-common but still supported way of configuration in October is to use a Your code however will only try the environment file if it exists, but won't write anything as there's no |
@bennothommo I've added functionality so it will check if the environment specific app config file exists and write the key there instead of only falling back to the main app config file. Do you think it's worth me adding checks to actually validate the app key line exists in that file as well (e.g. /config/dev/app.php)? |
@ericp-mrel That would be good. Ideally, the flow would be as follows:
|
@bennothommo I've added the necessary logic to properly check the environment specific app config file after the .env file check and then the fallback to the main app config file. I've also switched to using a regex for the key replacement in order to fix the issue where the app key wasn't being updated if it was originally empty since it was trying to perform a string replacement for an empty string on the entire file, which obviously doesn't result in any change. Let me know if there's any code you think I could clean up. Thanks |
Co-authored-by: Luke Towers <github@luketowers.ca>
Co-authored-by: Luke Towers <github@luketowers.ca>
Would it be possible to get some unit tests for each of the cases @bennothommo described? |
This allows the Config Writer to replace default values which are wrapped inside an env() call.
Sorry, I'm new to testing. How exactly would I write a test for this command without affecting the rest of the test since it requires creating/updating existing files (which could affect tests which ran after it). I tried looking at Laravel itself as a starting point to see how they do it, and they don't have a any tests for the key generate command either. |
@ericp-mrel You could check the tests I wrote for the |
I'd in fact suggest putting the tests in the main October repo as a separate PR - the |
Alright, I'll send a PR to the main repo, but is there anything special I need to do in order for the tests to actually work with the CI tests since these tests require another PR on the framework repo which isn't merge? |
Fixes an exception from being thrown when running tests about the env option not existing
@ericp-mrel don't stress too much about the CI on the separate repo - the tests will fail, but I'll manually test it with everything merged together when it's ready. |
See comment in octobercms/october#5496 |
Fixes octobercms/october#5496