Skip to content
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

Add enable_access_logs boolean config for swoole #894

Closed
wants to merge 3 commits into from

Conversation

tania-pets
Copy link

@tania-pets tania-pets commented May 16, 2024

Adds octane.swoole.enable_access_log config variable to toggle access logs for swoole server.
See #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
Copy link
Member

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

@taylorotwell taylorotwell marked this pull request as draft May 16, 2024 21:37
@tania-pets
Copy link
Author

tania-pets commented May 17, 2024

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 driesvints marked this pull request as ready for review May 17, 2024 07:35
@driesvints
Copy link
Member

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

@taylorotwell
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants