Validate skip_capture Classes

[rodnaph]

Related to #125, adds configuration validation that the classes passed to skip_capture exist.

[Jean85]

Hello there, and thanks for the contribution!
I appreciate the fix, but I think that the current solution is a bit "harsh". Using class_exists can make the whole app fail if the string is no longer a loadable class, like if the containing package got removed/updated; I would prefer a softer approach... Any suggestion? Maybe a regex, like /^\\?\w+(\\\w+)*$/?

[rodnaph]

Hey, thanks for the review. I don't have any "softer" ideas for how this could be handled sorry...

What brought me to add this patch was that I erroneously put in a class to match, and was looking to make this more fool-proof. For me, I'd prefer it to be as strict as possible so I was alerted of invalid configuration - but I understand if that's not the behaviour you want for this library.

[Jean85]

Well, I think that the regex should be enough, because the set of possible strings that are valid FQCN are pretty simple... Basically all the cases are:

• \SomeGlobalNamespacedClass
• \Some_PSR0_NamespacedClass
• \Some\Namespaced\Class

And other 3 without the starting \. You should rewrite your test with those 6 cases in the data provider and use a regex to validate it.

[mcfedr]

I guess the sense would be that if you put in a class that cannot be loaded, you probably made a mistake, crashes at container build time should be noticeable before you deploy

[stayallive]

Just a little thing to think about: I am not sure when that configuration is loaded but I assume on every request to Symfony so that could possibly add a lot of autoloading overhead.

[mcfedr]

The configuration is only run at compile time, this code wouldnt be run for every request

[stayallive]

Than you can ignore my comment completely 😄

[Jean85]

I guess the sense would be that if you put in a class that cannot be loaded, you probably made a mistake, crashes at container build time should be noticeable before you deploy

Yes, but that would be considered a BC; so I'm not inclined into implementing it like this in a minor/patch.

[Jean85]

Closing as stale. Feel free to reopen on top of master.