[WIP] Configuration Validation

[DonCallisto]

This fix #325

Is worthy to note two main things:

1. I have a failing test (regression?) that I just don't understand why is broken
2. I made all the handler cases specific (ie.: all parameters tested). I know that I can drastically reduce test lines by using a single provider and providing a specific invalid configuration but, in order to reach this, I needo to a) use acceptable parameters from the configuration (for example if I try to use foo as "additional" parameter the test will pass but only because foo will throw the same exception of "general acceptable" parameter but not accepted for the tested handler), b) if I use accetable parameters as explained in a) I had to make a choice to use all of them or just a random one that's not accepted. I've done all of them but if you prefer to have just a "random one" please let me know and explain why so I can modify it properly.

Thanks for any feedback.

[DonCallisto]

Moreover, I left this comment as a reminder for myself, maybe also the changelog (and the docs?) should be updated along with this PR.

[Seldaek]

Could this be generated by recursively merging $this->acceptedParamsByHandlerType instead? So it doesn't need to be kept up to date, because I'm pretty sure it'll get outdated otherwise.

[Seldaek]

I don't think so? I don't think there is an expectation that this stuff is case insensitive generally speaking, but I might be mistaken.

[Seldaek]

Similarly to the note below, strtolower here and at line 1080 probably should be removed.. types were not case insensitive until now?

[Seldaek]

I think this should rather throw if type is missing? AFAIK there is no valid handler config without a type. Same below if the type is unknown I'd say throw instead of returning a valid value

[DonCallisto]

By returning $v, I let other validations to take place. Here I'm validating only if handler type has correct options, so I think it pretty safe to return $v directly. WDYT?

[Seldaek]

Yup sorry for the rant, sometimes it's one too many and someone gets shouted at, not always deservedly so..

Anyway reviewed the PR now as penance.. The test class IMO is way too long and way too much code is spent testing invalid configs which are not really as important as valid ones. Now that the code is there though I don't know if there is much value in removing it.. I just wish it could be done more concisely and perhaps more automated, like generating those invalid config tests using a smarter provider which reads from the available params perhaps having one single provider for valid configs would help there too to be able to array_diff against that in the invalid test provider. Instead of having to write a test+provider per handler type.

[Seldaek]

As for the test failure, I think it's an invalid config in the existing fixtures, https://github.com/symfony/monolog-bundle/blob/master/Tests/DependencyInjection/Fixtures/xml/handlers_with_channels.xml#L33 you should remove the handler="nested" part I think that'll fix it.

[DonCallisto]

Yup sorry for the rant, sometimes it's one too many and someone gets shouted at, not always deservedly so..

Nevermind. Really. Sometime happens and realizing it is more important that the debate. Don't worry.

The test class IMO is way too long and way too much code is spent testing invalid configs which are not really as important as valid ones. Now that the code is there though I don't know if there is much value in removing it.. I just wish it could be done more concisely and perhaps more automated, like generating those invalid config tests using a smarter provider which reads from the available params perhaps having one single provider for valid configs would help there too to be able to array_diff against that in the invalid test provider.

Yeah, indeed was one of my biggest worries about this PR. I can think of a more concise way to reach this. Meanwhile I've done the other requested things. Gonna push them in a separate and WIP commit.

Thanks for having reviewed this and thank you for your time.

[DonCallisto]

I've modified tests (don't pay attention to variable name and functions name, I'll refactor if that's a good approach to you).
One thing is worth to notice: some handlers can manage multiple configurations (like finger_crossed). In such cases, the only way to achieve both test's paths (correct configuration one and wrong one) is to keep, only for those handlers that are three (check the test file), old test code.
Otherwise we can remove all wrong configuration tests but I'm not sure this will prevent someone to change the code and mess the things up with validation.