search

php-pm / php-pm-httpkernel

HttpKernel adapter for use of Symfony and Laravel frameworks with PHP-PM
MIT License
246 stars 72 forks source link

[Laravel] phppm should not modify env that related to laravel behavior #98

Closed wanghanlin closed 4 years ago

wanghanlin commented 6 years ago

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

marcj commented 6 years ago

Why not? This is set when you use ppm start --debug 1 argument. Why don't you just use that argument?

wanghanlin commented 6 years ago

Hi @marcj here are my personal thoughts on this.

  1. 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 when APP_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.
  2. 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.
  3. in Symfony http kernel bridge, we don't have this kind of code to override behaviour, I would suggest we keep same behaviour here.
marcj commented 6 years ago

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.

wanghanlin commented 6 years ago

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

acasademont commented 4 years ago

Hi @wanghanlin how can we move forward with this? Is it still relevant?

wanghanlin commented 4 years ago

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!