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

[FrameworkBundle] framework.php_errors.log now accept a log level #26504

Merged
merged 1 commit into from Mar 27, 2018

Conversation

Projects
None yet
5 participants
@Simperfit
Contributor

Simperfit commented Mar 13, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26267
License MIT
Doc PR todo

We are testing that framework.php_errors.log is either a bool or an int (set the value of the log level you want).
This fixes #26267, so it gives a way to not log some level on production.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 13, 2018

would be great to turn the framework.php_errors.log config entry into a "bool|int", and when an "int" is provided, use the value to set the parameter

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Mar 13, 2018

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Mar 14, 2018

@nicolas-grekas I have changed this PR, could you please guide me on how to write test for this ?

@Simperfit Simperfit changed the title from [FrameworkBundle] add a parameter for the debug production log level to [FrameworkBundle] framework.php_errors.log now accept a log level Mar 14, 2018

@nicolas-grekas

for tests, please check the existing ones, I don't have specific ideas on the topic sorry

@@ -601,10 +601,14 @@ private function registerDebugConfiguration(array $config, ContainerBuilder $con
$definition = $container->findDefinition('debug.debug_handlers_listener');
if (!$config['log']) {
if (\is_bool($config['log']) && !$config['log']) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 14, 2018

Member

if (!$config['log']) { is enough: 0 also disables the logger

$definition->replaceArgument(1, null);
}
if (\is_int($config['log'])) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 14, 2018

Member

if (\is_int($config['log']) && $config['log']) {

@@ -833,7 +833,7 @@ private function addPhpErrorsSection(ArrayNodeDefinition $rootNode)
->info('PHP errors handling configuration')
->addDefaultsIfNotSet()
->children()
->booleanNode('log')
->scalarNode('log')

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 14, 2018

Member

maybe forbid anything else than a bool|int?

This comment has been minimized.

@Simperfit

Simperfit Mar 26, 2018

Contributor

done

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Mar 26, 2018

Status: Needs Review

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Mar 26, 2018

Tests Fixed.

->info('Use the app logger instead of the PHP logger for logging PHP errors.')
->defaultValue($this->debug)
->treatNullLike($this->debug)
->validate()
->ifTrue(function ($v) { return !(\is_int($v) || \is_bool($v)); })
->thenInvalid('The "php_errors.log" parameter should be either a int or a boolean.')

This comment has been minimized.

@fabpot

fabpot Mar 27, 2018

Member

either an integer or a boolean?

This comment has been minimized.

@Simperfit

Simperfit Mar 27, 2018

Contributor

@fabpot updated.

->info('Use the app logger instead of the PHP logger for logging PHP errors.')
->defaultValue($this->debug)
->treatNullLike($this->debug)
->validate()
->ifTrue(function ($v) { return !(\is_int($v) || \is_bool($v)); })
->thenInvalid('The "php_errors.log" parameter should be either a integer or a boolean.')

This comment has been minimized.

@fabpot

fabpot Mar 27, 2018

Member

an integer :)

This comment has been minimized.

@Simperfit

Simperfit Mar 27, 2018

Contributor

This is good now :p.

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 27, 2018

Thank you @Simperfit.

@fabpot fabpot merged commit 664f821 into symfony:master Mar 27, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 27, 2018

feature #26504 [FrameworkBundle] framework.php_errors.log now accept …
…a log level (Simperfit)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[FrameworkBundle] framework.php_errors.log now accept a log level

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #26267   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        |  todo <!-- required for new features -->

We are testing that `framework.php_errors.log` is either a bool or an int (set the value of the log level you want).
This fixes #26267, so it gives a way to not log some level on production.

Commits
-------

664f821 [FrameworkBundle] framework.php_errors.log now accept a log level

@Simperfit Simperfit deleted the Simperfit:feature/add-a-parameter-for-log-level branch Mar 30, 2018

nicolas-grekas added a commit that referenced this pull request Apr 9, 2018

bug #26874 [FrameworkBundle] Fixed configuration of php_errors.log (l…
…yrixx)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[FrameworkBundle] Fixed configuration of php_errors.log

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #26504 and #26740 (wrong implementation)
| License       | MIT
| Doc PR        |

Commits
-------

8e0fcf9 [FrameworkBundle] Fixed configuration of php_errors.log

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

@@ -833,10 +833,14 @@ private function addPhpErrorsSection(ArrayNodeDefinition $rootNode)
->info('PHP errors handling configuration')
->addDefaultsIfNotSet()
->children()
->booleanNode('log')
->scalarNode('log')
->info('Use the app logger instead of the PHP logger for logging PHP errors.')

This comment has been minimized.

@bendavies

bendavies May 29, 2018

Contributor

is this really descriptive now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment