Allow processors' options to be override by command line arguments

[raphael-trzpit]

It is possible to override defaults options with command line arguments. But when you add 'extras' to change the default value for your consumer, this disable the command line arguments override, because the default extras are merged first. This change aims to keep the override even if you set extra options in your config.

[raphael-trzpit]

i'm gonna do it today or tomorrow when i got some time :)

[raphael-trzpit]

tests done, ping @olaurendeau :)

[odolbeau]

Hi!

I tooks me a lot of time to understand your problem. :)

Let's be clear on how it works today. First of all here is the priority we want to respect: command options > extra config > default

Let's take a concrete example with the MaxExecutionTimeProcessor which have a max_execution_time configurable option. In this example the "default value" we talk about is the value displayed when you print the help for the command and the "actual value" is what you'll have in your processor in the $options['max_execution_time']..

• ✅ Without extra nor command option, the default value is 300 (as defined in the configurator, at the line you pointed out). As it's not overridden anywhere, the actual value is also 300.
• ✅ With extra value (500) but without command option, the new default value is 500. As it's not overridden by a command option, the actual value is also 500.
• ✅ Without extra but with a command option (100), the default value is 300 but the actual value is 100.
• ✅ With extra (value: 500) and with a command option (100), the default value is 500 but and actual value is 100.

This is the latest use case which don't work in your case if I understand well.

Here is the config I used to check that everything is fine:

swarrot:
    consumers:
        test:
            processor: "AppBundle\\Processor\\DumpConfigProcessor"
            middleware_stack:
                - configurator: swarrot.processor.max_execution_time
                  extras:
                      max_execution_time: 100

Here is the config you (probably) used:

swarrot:
    consumers:
        test:
            processor: "AppBundle\\Processor\\DumpConfigProcessor"
            middleware_stack:
                - configurator: swarrot.processor.max_execution_time
            extras:
                max_execution_time: 100

I added extras for the configurator, you added it on the consumer directly. It took me some time to find this problem... even if I already encountered it with a teammate. I'm embarrassed, I didn't take the time to correct this...

Anyway, what should we do now? IMO it should not be possible to add an invalid configuration key somewhere. Right now:

• ✅ if you add an unknown key in the extras part of your consumer: you have an exception thrown by the OptionsResolver.
• ❌ if you add an unknown key in the extras part of a processor: the option is not available in your processor but you don't have any exception.
• ❌ if you add a key used by a processor in the extras part of your consumer: this is a mess (that's exactly the problem you encounter)

Here is what I propose:

• First of all, I'll be glad if you could create a new PR with your amazing tests! ❤️ Thanks a lot for this, we definitively need to increase the code coverage of this bundle and this is a huge step. (thanks also for your provider! I didn't know it was possible to "name" each entry. :) )
• We need to correct this extras config problem. I don't know how right now and I will be too busy drinking a beer once this message will be sent to investigate. :) I should have some time next week to take a look at this.
• We need to add tests to check that all processor respect the configuration priority (Command option > extra config > default). Spoiler: this is not the case for now (see AckProcessorConfigurator. OK it's a big deal as I doubt anyone set requeue_on_error to true in extra but still)

[raphael-trzpit]

hey, i agree the 3rd case is hard : options can be added to both the extras of the consumer and the extras of the processor (because they are merged). Currently the extras of the processor are merged first, so any configuration done in the processorConfigurators doesn't even matter (vs the processor).

I made a PR for the command tests :)