laravel / octane

Supercharge your Laravel application's performance.
https://laravel.com/docs/octane
MIT License
3.71k stars 284 forks source link

Add enable_access_logs boolean config for swoole #894

Closed tania-pets closed 1 month ago

tania-pets commented 1 month ago

Adds octane.swoole.enable_access_log config variable to toggle access logs for swoole server. See https://github.com/laravel/octane/issues/893

**It defaults to true, that means it will enable the logs for non-local environments if it's not set to false explicitly in the config file.

**It defaults to false, that means it won't enable the logs for non-local environments, but it will stop logging for local/testing

taylorotwell commented 1 month ago

Since it defaults to true would this also enable it in production for existing apps?

tania-pets commented 1 month ago

Since it defaults to true would this also enable it in production for existing apps?

@taylorotwell Exactly, yes. In order to maintain the current behaviour, we could do it like:

if (! $sandbox->environment('local', 'testing')) {
    if (! config('octane.swoole.enable_access_logs', false)) {
        return;
    }
}

That way the logs will be always enabled for local/testing and the config will control non-local only. The config variable name will be misleading though, because it will have no effect on local/testing. We could rename it to something like enable_non_local_access_logs or something, but I think it's much better to be able to control this per environment, regardless if it's local or not (There can be multiple non local environments, and each should be configurable)

But I do get the concern about changing production behaviour.

So maybe it's better to just default it to false:

if (! config('octane.swoole.enable_access_logs', false)) {
    return;
}

This ^ will switch off the logs for local/testing, but changing such a local behaviour is less concerning than changing production. PR updated.

driesvints commented 1 month ago

@tania-pets just marking this as ready again so Taylor sees your response

taylorotwell commented 1 month ago

Sorry, this also changes the behavior as now they won't be enabled on local dev.