[Laravel] phppm should not modify env that related to laravel behavior
wanghanlin opened this issue · 6 comments
https://github.com/php-pm/php-pm-httpkernel/blob/master/Bootstraps/Laravel.php#L38
this line will modify the behaviour of a .env file developer set, I think phppm especially a httpkernel bridge shouldn't modify the default behaviour of laravel.
ref: php-pm/php-pm#363
Why not? This is set when you use ppm start --debug 1
argument. Why don't you just use that argument?
Hi @marcj here are my personal thoughts on this.
- As mentioned in issues in php-pm repo, I think there are cases that user want to enable hot-reloading, but not set
APP_DEBUG
to true, the underlying reason here is whenAPP_DEBUG
is true, laravel will now by default display all ENV (in old laravel it won't), so all credentials like database password will be exposed, we don't have documentation in php-pm that explain this behaviour and highlight to users, so it also bring a potential security reason that developer may not aware of. - As the project php-pm is, the main purpose is to act like a http server with event driven reactphp, so it's not appropriate to me to override some behaviour of how laravel works.
- in Symfony http kernel bridge, we don't have this kind of code to override behaviour, I would suggest we keep same behaviour here.
Good arguments, then let's change that accordingly. Maybe it's best to remove the debug option, and trigger the hot-reload functionality behind a new option.
do you need prevent breaking change? if so maybe we can add a new option --disable-env-override
or something and default to false. but it's hard to do non-breaking change in PHPPM\Bootstraps\Laravel so i'm not sure what's best here
Hi @wanghanlin how can we move forward with this? Is it still relevant?
Hi @acasademont, I haven't been using this for a while, but I just checked the latest code seems don't have same issue, let's just close this for now and if someone else face this issue, we can open again. Thanks!