-
Notifications
You must be signed in to change notification settings - Fork 7
Added command to migrate config.json files to .env variables #288
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions in comments.
Most important is the issue of default values. The convention for .env[.*]
in Symfony applications is that default values are defined in .env
and overrides in .env.local
.
This command defines defaults and asks that you set them in .env.local
. It would be better to only give out put for lines with values set. If no value is set, the variable should be omitted to allow fallback to defaults in .env
|
||
$content = file_get_contents($input->getArgument('filepath')); | ||
|
||
$config = json_decode($content, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use JSON_THROW_ON_ERROR
to force an exception if json not valid. Then add error/command failed
return Command::INVALID; | ||
} | ||
|
||
$content = file_get_contents($input->getArgument('filepath')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try/catch when reading file
} | ||
|
||
$env = "###> Admin configuration ###\n"; | ||
$env .= 'ADMIN_REJSEPLANEN_APIKEY="'.$rejseplanenApiKey."\"\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why output ADMIN_REJSEPLANEN_APIKEY
if it's empty?
$env .= 'ADMIN_ENHANCED_PREVIEW='.$enhancedPreview."\n"; | ||
$env .= "###< Admin configuration ###\n"; | ||
|
||
$output->writeln($env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to just $output->writeln()
one line at a time instead of building a multi line variable.
(Don't know how new line works in the console across platforms, but normally you should use PHP_EOL
to match the platform. Recommend letting $output->writeln()
handle line breaks.)
$env .= 'CLIENT_DEBUG='.$debug."\n"; | ||
$env .= "###< Client configuration ###\n"; | ||
|
||
$output->writeln($env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above about multi lines
Co-authored-by: Ture Gjørup <[email protected]>
Co-authored-by: Ture Gjørup <[email protected]>
…ntln to output line
…-api-service into feature/upgrade-guide
…-api-service into feature/upgrade-guide
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should potentially target the 2.* branch so that you can run it prior to upgrading to 3.0
Issue
#249
Description
Checklist