Skip to content

Conversation

@dzuelke
Copy link
Contributor

@dzuelke dzuelke commented Nov 9, 2014

Unlike other settings, this has a perfectly reasonable default, calculated using
a dynamic formula. If the default was hardcoded to "2" or something, then it
would make sense to have a warning, since that could potentially be bad, but for
a dynamically calculated value based on other mandatory settings, a notice ought
to be enough.

Unlike other settings, this has a perfectly reasonable default, calculated using
a dynamic formula. If the default was hardcoded to "2" or something, then it
would make sense to have a warning, since that could potentially be bad, but for
a dynamically calculated value based on other mandatory settings, a notice ought
to be enough.
@remicollet
Copy link
Member

Seems ok to me.

Can you please open a bug to link this change with.

@remicollet
Copy link
Member

Having a unit test will also be appreciated

@dzuelke
Copy link
Contributor Author

dzuelke commented Nov 19, 2014

Okay so this fix doesn't actually work.

The reason is that the warning is emitted in fpm_conf_process_all_pools(), which is called in fpm_conf_post_process(), which is called in fpm_conf_init_main(), which is called right before fpm_unix_init_main().

I propose to move the call to zlog_init(fpm_globals.log_level) from fpm_unix_init_main() to fpm_conf_post_process() right after line 1161:

fpm_globals.log_level = fpm_global_config.log_level;

Does that sound reasonable, @remicollet ?

@dzuelke
Copy link
Contributor Author

dzuelke commented Nov 19, 2014

Alternatively, leave it in both places. Either way this kind of renders the other (already merged) PR obsolete, but no harm done I guess.

@dzuelke
Copy link
Contributor Author

dzuelke commented Nov 19, 2014

Or should I re-open that other PR with an amended fix, and we leave this one here separate? Harder to test and merge though. Please advise.

@remicollet
Copy link
Member

Best way is to provide all needed changes in this PR (the other is merged/closed)

@dzuelke
Copy link
Contributor Author

dzuelke commented Nov 19, 2014

Yup, just saying that this change moves the location of where zlog_set_level() is called yet again, so the other PR is technically redundant.

from fpm_unix_init_main() to fpm_conf_post_process() this time (see php#894),
because otherwise nothing in fpm_conf_init_main() obeys log levels
@dzuelke
Copy link
Contributor Author

dzuelke commented Nov 19, 2014

PR updated with newly moved zlog_set_level() call and a test

@dzuelke
Copy link
Contributor Author

dzuelke commented Nov 19, 2014

php-pulls pushed a commit that referenced this pull request Nov 20, 2014
php-pulls pushed a commit that referenced this pull request Nov 20, 2014
@php-pulls
Copy link

Comment on behalf of remi at php.net:

merged

@php-pulls php-pulls closed this Nov 20, 2014
@remicollet
Copy link
Member

Thanks for your contribution.

@dzuelke
Copy link
Contributor Author

dzuelke commented Nov 20, 2014

Thanks @remicollet. I was wondering the same about the tests (have one in warning level and one in notice) and meant to ask you if I should do that but forgot :)

@dzuelke
Copy link
Contributor Author

dzuelke commented Nov 20, 2014

Could you also merge this to PHP-5.5 please, like the other change? Thanks @remicollet!

@remicollet
Copy link
Member

Merging in 5.5 is not possible for test (rely on -O which is 5.6+)
But will think about the code changes...
But we already have so much change in fpm.... :(

php-pulls pushed a commit that referenced this pull request Nov 20, 2014
* PHP-5.6:
  Raise a warning when listen = hostname used and is resolved as multiple addresses
  NEWS
  Fixed #68458 Change pm.start_servers default warning to notice
  fix test description
  tests for #895
  move zlog_set_level() again
  Change pm.start_servers default warning to notice
@remicollet
Copy link
Member

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