8000 Add ability to configure the PsrLogMessageProcessor by HypeMC · Pull Request #417 · symfony/monolog-bundle · GitHub
[go: up one dir, main page]
Skip to content

Add ability to configure the PsrLogMessageProcessor #417

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 10, 2022
Merged

Add ability to configure the PsrLogMessageProcessor #417

merged 3 commits into from
May 10, 2022

Conversation

HypeMC
Copy link
Member
@HypeMC HypeMC commented Sep 20, 2021 •
edited
Loading

Adds the ability to configure the PsrLogMessageProcessor using semantic configuration.

See Seldaek/monolog#940, Seldaek/monolog#1046, Seldaek/monolog#1487.

8000
@HypeMC HypeMC force-pushed the psr-processor-options branch from 795bb94 to c0ea419 Compare September 21, 2021 04:51
@HypeMC HypeMC force-pushed the psr-processor-options branch from c0ea419 to 0d5b6a2 Compare September 28, 2021 21:01
@Seldaek Seldaek merged commit 4694fc2 into symfony:master May 10, 2022
@Seldaek
Copy link
Member
Seldaek commented May 10, 2022

Thanks

8000
@HypeMC HypeMC deleted the psr-processor-options branch May 10, 2022 13:56
@Jeroeny
Copy link
Jeroeny commented May 11, 2022

Hi, was it known that this was a Backwards breaking change ?

Given the following configuration in PHP:

// ./config/packages/monolog.php

use Symfony\Config\MonologConfig;

return static function (MonologConfig $config): void {

    $config->handler('console')
        ->type('console')
        ->processPsr3Messages(false)
        ->channels()
            ->elements(['!event', '!doctrine']);
};

Which the signature of processPsr3Messages being: public function processPsr3Messages($value): static.

Updated to 3.8:

$ .\bin\console                        
TypeError {#4198
  #message: "Symfony\Config\Monolog\HandlerConfig::processPsr3Messages(): Argument #1 ($value) must be of type array, bool given, called in .\config\packages\dev\monolog.php on line 19"
  #code: 0
  #file: ".\var\cache\dev\Symfony\Config\Monolog\HandlerConfig.php"
  #line: 229
  trace: {
    .\var\cache\dev\Symfony\Config\Monolog\HandlerConfig.php:229 {
      Symfony\Config\Monolog\HandlerConfig->processPsr3Messages(array $value = []): ProcessPsr3MessagesConfig
      ›
      › public function processPsr3Messages(array $value = []): \Symfony\Config\Monolog\HandlerConfig\ProcessPsr3MessagesConfig
      › {
    }

You can see the new method signature in the trace. I had to change the config to:

// ./config/packages/monolog.php

use Symfony\Config\MonologConfig;

return static function (MonologConfig $config): void {

    $console = $config->handler('console')
        ->type('console');
    $console->processPsr3Messages()
        ->enabled(false);
    $console->channels()
        ->elements(['!event', '!doctrine']);
};

@Seldaek
Copy link
Member
Seldaek commented May 11, 2022

Ah that's quite a surprising problem to me.. Because as I understand it this solves the BC aspect for yaml/xml/..

                    ->beforeNormalization()
                        ->ifTrue(static function ($v) { return !\is_array($v); })
                        ->then(static function ($v) { return ['enabled' => $v]; })
                    ->end()

I am not sure if there is anything we can do to get the generated config accept booleans too?

@Jeroeny
Copy link
Jeroeny commented May 11, 2022

@Seldaek I noticed that too, but in PHP configs it would never be not an array due to the typehint. And ->variableNode
is probably not usable either, because then you can't use the arrayNode specific addDefaultsIfNotSet and such..

@HypeMC
Copy link
Member Author
HypeMC commented May 11, 2022

I didn't take the new config builder into consideration back when I initially made this PR 😞
I guess the best way to go about this would be to add a new process_psr_3_messages_options & keep process_psr_3_messages as it was before.

@Seldaek
Copy link
Member
Seldaek commented May 11, 2022

I had a quick chat with @nicolas-grekas and he agrees this is a bug in the config generator if it does not allow people to migrate configs in a BC way.. So ideally the parameter type should be removed from https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Config/Builder/ConfigBuilderGenerator.php#L132 and perhaps other places, if anyone has time to do a PR there.

I think it might be more sensible to only remove the parameter types if normalization is active on that node (which indicates some sort of BC/type juggling might be involved), but I am not sure if that's possible at all.

@HypeMC
Copy link
Member Author
HypeMC commented May 11, 2022

I can do the PR, will open it some time today.

@jderusse
Copy link
Member

If I'm not wrong, this PR symfony/symfony#44166 should have handled that case

@HypeMC HypeMC mentioned this pull request May 11, 2022
Merged
@HypeMC
Copy link
Member Author
HypeMC commented May 11, 2022

PR opened symfony/symfony#46328

nicolas-grekas added a commit to symfony/symfony that referenced this pull request May 17, 2022
@nicolas-grekas
bug #46328 [Config] Allow scalar configuration in PHP Configuration (…
56c6dc1 
…jderusse, HypeMC)

This PR was merged into the 5.4 branch.

Discussion
----------

[Config] Allow scalar configuration in PHP Configuration

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix symfony/monolog-bundle#417 (comment)
| License       | MIT
| Doc PR        | -

Fixes passing scalar values to array nodes that have a `beforeNormalization` hook, eg:

```php
return static function (MonologConfig $config): void {
    $config->handler('console')
        // ...
        ->processPsr3Messages()
            ->enabled(true)
    ;
};
```

can be shortened thanks to a [`beforeNormalization` hook](https://github.com/symfony/monolog-bundle/blob/a41bbcdc1105603b6d73a7d9a43a3788f8e0fb7d/DependencyInjection/Configuration.php#L453):

```php
return static function (MonologConfig $config): void {
    $config->handler('console')
        // ...
        ->processPsr3Messages(true)
    ;
};
```

I've used some of the code from #44166 by `@jderusse`. Since his PR is a feature it can't go on 5.4, but it still helped a lot.

Commits
-------

2d81a3a [Config] Allow scalar configuration in PHP Configuration
1c176e1 [Config] Allow scalar configuration in PHP Configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HypeMC @Seldaek @Jeroeny @jderusse
0