strange configuration parsing of monolog, ??? BC-break after release of 3.4.0 ???

[boris-brtan]

symfony: ^3.4
monolog: 3.4.0

in the case i have something like this

monolog:
    channels: ["cron", "slow"]
    handlers:
        main:
            type: fingers_crossed
            buffer_size: 1000
            action_level: warning
            handler: nested
            excluded_404s:
                - ^/
        nested:
            type: stream
            path: "/tmp/logpipe"
            level: debug
        cron:
            type: stream
            path: "/tmp/logpipe"
            channels: cron
            level: debug

after merge of #271 monolog config is invalid because the parsed configuration is treated as group type instead of fingers_crossed

[PetrHeinz]

The problem is that in app/config/packages/monolog.yml we have this handler definition:

monolog:
    channels: ["cron", "slow"]
    handlers:
        main:
            type: fingers_crossed
            buffer_size: 1000
            action_level: warning
            handler: nested
            excluded_404s:
                - ^/
        nested:
            type: stream
    # ...

and in app/config/packages/dev/monolog.yml we change the default fingers_crossed handler to group type:

monolog:
    handlers:
        main:
            # change "fingers_crossed" handler to "group" that works as a passthrough to "nested"
            type: group
            members: [ nested ]

I've failed to empty the excluded_404s configuration from inside the app/config/packages/dev/monolog.yml, as the array items only get appended (I've tried excluded_404s: ~ and excluded_404s: [] to no avail).

Not sure if I'm missing something obvious or this change really broke our config. If so, we'd have to stop changing the type of a specific handler for different environments like we do now.

[PetrHeinz]

I've been missing something. An mergable configuration can be unset using excluded_404s: false

[PetrHeinz]

The change in #271 might not be a BC break per-se, but it can complicate things for the users as seen above. Personally, I'm in favor of being a bit more restrictive in configuration and rather throw an exception when an invalid state is detected, so I don't mind the complications as they force users to correct usage...

[COil]

Looks like the same type of error after 3.4 upgrade :

Invalid configuration for path "monolog.handlers.slack": You can only use excluded_http_codes/excluded_404s with a FingersCrossedHandler definition                                                                                                                                                       
In ExprBuilder.php line 189:
You can only use excluded_http_codes/excluded_404s with a FingersCrossedHandler definition

[boris-brtan]

+1

[SebLevDev]

+1

[lyrixx]

I managed to reproduce it. Everything is well explained in this comment

And the following fix makes your application working (and more robust)

I've been missing something. An mergable configuration can be unset using excluded_404s: false

thanks @PetrHeinz

---

I don't think we can do anything here. using ->performNoDeepMerging() will break too many applications.

So you should unset the configuration that are not compatible with your handler:

monolog:
    handlers:
        main:
            # change "fingers_crossed" handler to "group" that works as a passthrough to "nested"
            type: group
            members: [ nested ]
            excluded_404s: false

[TamasSzigeti]

I have the same issue with excluded_http_codes and overriding with false doesn't help, same result as with ~ or []

[lyrixx]

Could you create a reproducer? Thanks

[boris-brtan]

OK, so the result is that it is not a part of the BC-promise or it is importnant BC-break that should be introduced also in minor?

[lyrixx]

This is not covered by the BC promise

[boris-brtan]

now i see, OK THX.

[COil]

I don't get the point, it breaks existing applications but it's not considered as a BC break ? I understand it can be easily fixed, but what the point of using semver in this case.

[boris-brtan]

as i understand it and also experienced it is a BC-break if it is said it is. You can check the BC-promise list.
and since i am agile and open-source i 100% trust @lyrixx when writes it is not in BC-promise

[lyrixx]

@COil Your usage of MonologBundle was incorrect, and someone added a check to ensure the configuration was correct.
We have documented Our Backward Compatibility Promise. I may be wrong, but PR #271 did not violate this promise, isn't?

[TamasSzigeti]

Here's the reproducer
https://github.com/TamasSzigeti/monolog_bundle_reproduce_309/commits/master

TBH it's not a big deal, it's cleaner to define monolog configs separately per env anyway

[lyrixx]

@TamasSzigeti Thanks, I will have a look ASAP

[PetrHeinz]

@lyrixx Thanks for the explanation of your point of view on BC, I tried to explain it similarly in #309 (comment). 👍

Almost every change in code can break some applications. That's why it's important to have a BC Promise.

[COil]

ok, ok. I'll give a try this weekend for one of my project. Thank you.

[lyrixx]

@TamasSzigeti

You application can not boot:

In Configuration.php line 402:
                                                                         
  Warning: array_map(): Expected parameter 2 to be an array, bool given  

---

Anyway, let take a step back on this issue.

What do you (all) have in mind when you configure monolog globally (config/packages/monolog.yaml) then per env (config/packages/dev/monolog.yaml) ?
What do you think symfony should do // What did you expect ?

For now, symfony process configuration like following:

It loads the global configuration first:

monolog:
    handlers:
        main:
            type: fingers_crossed
            action_level: error
            handler: nested
            excluded_http_codes: [404, 405]

Then it (try to) merges it with the env configuration:

monolog:
    handlers:
        main:
            type: stream
            path: "%kernel.logs_dir%/%kernel.environment%.log"
            level: debug
            channels: ["!event"]
            excluded_http_codes: false

So this results in something really strange. Which configuration should win? The last one? Why?
This could be determined by the kernel though:

    protected function configureContainer(ContainerBuilder $container, LoaderInterface $loader): void
    {
        // ...
        // global
        $loader->load($confDir.'/{packages}/*'.self::CONFIG_EXTS, 'glob');
         // per env
        $loader->load($confDir.'/{packages}/'.$this->environment.'/**/*'.self::CONFIG_EXTS, 'glob');

But anyway, what should be done? Should we drop all the previous configuration if a new configuration is met with the same name?

[TamasSzigeti]

Exactly, "Expected parameter 2 to be an array" shows on false, and ~ or [] gives you the "You can only use excluded_http_codes/excluded_404s blabla".

As I said, I feel it's better to config it per env, which I have done in our project. Much cleaner.

Otherwise yes, the env-specific config should override the global one. The problem here is that it doesn't, null or empty array doesn't override the global one, and false just breaks, as you saw it.

[lyrixx]

BTW, by default monolog does not have global configuration, exactly for this
https://github.com/symfony/recipes/tree/master/symfony/monolog-bundle/3.1/config/packages

[TamasSzigeti]

Correct, it affected our legacy pre-flex project.