[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.

[DonCallisto]

@Seldaek did you had the chance to take a look at my questions and to this PR?

[Seldaek]

Yes I had lots of free time so I took a look and then because I am an idiot I decided not to write anything nor to act on that, and just moved on with my life.

[DonCallisto]

@Seldaek Sorry to read this.
I was just asking in a polite way seven months after I've been doing this. Was wondering if that was just "backlogged", if it was considered "useless", or whatever; just wish I had an update, nothing more. No prossure with my question.
Take this just as a reminder that you (of course) could have answered in a lot of way, but not harshly as you did.
Sorry for the misunderstanding but, from my standpoint, that's not an acceptable reply.

[Seldaek]

From my standpoint it also was not an acceptable intrusion in my inbox.. Hence the harsh reply. I know fully well that there's a ton of issues/PRs rotting away here, it sucks and I hate it but I don't have time. I'm focusing on getting Composer 2 out of the door. This project has other people able to merge stuff so I'm hoping someone else will take care of things.

[DonCallisto]

Ok, let me explain.
With my question I didn't intended to put pressure on you.
I wasn't complaining about anything (as you can see).
From time to time (seven months is a "good" temporal span) when I can spend some time on OSS contribution, I make a recap to see if there's something old I was working on and that is still waiting for some reasons. So, since yesterday and for the next days on, I would have been able to work on OSS.
Maybe the question about this test could have been answered in two minutes, maybe not; actually I don't know, so I was politely asking.
My question wasn't a sneaky way to do anything but a question. That's it.
If someone opens a new PR, would you consider it an "invasion" of your inbox?
If someone opens a new issue?
If someone tags you on twitter?
And so on, so on, so on...
What make the difference, here, is the attitude and the insistence of people, I was not harsh at all nor insistent.

How many times I've heard

"don't open an issue and ask for others to do the work. just contribute yourself"

Well, it's just what I did.
After that, knowing that everyone has a life, has other projects, has litte free time, I just waited to know something. And that's part of the game, so it was really natural to me. Also asking politely was natural, considering it was my first question about this.
Let's pretend for a moment I'm a first time OSS contributor overall. Wasn't you over reacting? Wasn't you not welcoming someone, here, with your behavior?

Ok, shit happens and the point is not who is right or who is wrong, I'm just reflecting out loud.
I can imagine how little your free time is, how much effort your putting in composer 2 (that's more "critical" than this library, of course) and all the things, but I was just asking.
Just a polite question.

Apart that, I really admire your work and the time you spend on OSS. Composer is great and I'm sure Composer 2 is a big challange. I didn't intended to put pressure on you. I'm honest. If I contribute to OSS is thanks to you, Ocramius, Potencier, etc.
I'm surprised because I saw you to be really polite and that over reaction was a huge shock to me.
But I'm pretty sure you didn't understood my intentions with that question, and, hopefully, now is more clear.

Thanks again for all the effort and the work.

[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.