Validating configuration #325
Description
We should try to enforce user's correct configuration as commented here.
Why? Because I've found a tricky misconfiguration that leaded to wrong parsing of my logs and in loosing them.
That was the (wrong) configuration stripped off the not congruous parts in order to avoid noise, please don't focus on meaning but keep attention on technical matters
monolog:
channels: ['foo_channel']
handlers:
foo_channel_handler:
channels: [foo_channel]
type: stream # <--- THE "SILENT ERROR"
level: error
handler: grouped
grouped:
type: group
members: [streamed]
streamed:
type: stream
path: "%kernel.logs_dir%/%kernel.environment%.log"
level: debug
formatter: monolog.formatter.session_request
monolog.formatter.session_request:
class: Monolog\Formatter\LineFormatter
arguments:
- "[%%datetime%%] [%%extra.token%%] %%channel%%.%%level_name%%: %%message%% \n"
This configuration is clearly wrong (type: stream tries to use filter wrapper handler for a configuration error) but Symfony didn't raise an exception.
Why this could be a problem?
In this particular case I had two default values explicitated:
path --> https://github.com/symfony/monolog-bundle/blob/master/DependencyInjection/Configuration.php#L377
formatter (I mean, the Class) --> Monolog\Formatter\LineFormatter (AFAIK is the default one)
So in my tests the log was created at the right location using the desired formatter, except that I dind't noticed that [%%extra.token%%] was never included. This is due to the fact that, correctly, stream type never "pass" control to grouped and so to streamed.
That was nasty to debug and lost several hours.
We have two chances, both seems to introduce a BC break:
- Handlers
typeas configuration keys
Specifying the type of the handler as key and checking that at least one of the the types is specified under handler's name we can better state what to expect and what should not be nested there. One of the things that worry me the most with this approach is we should carry every acceptable value under each type key in the configuraton three.
Moreover this would introduce a new way of declaring (and so checking) custom handlers, something like "force" somehow the user to give a definition for its type.
- Performing semantic checks in
MonologExtension
This seems the best way from my standpoint. We can easily check here that all expected attributes (and no more that those) are carried by the configuration, raising and error otherwise.
I think that both methods introduce a BC break as in the first case everyone needs to change configuration files whereas in the latter case we were accepting wrong configuration that could possibly not working anymore.
If this makes sense to you I'm willing to work in order to reach what I've shown above.
I'm also open to other ideas.
Let me know what do you think.
Metadata
Metadata
Assignees
Labels
Type
Projects
Milestone
Relationships
Participants
Activity
Seldaek commentedon Oct 6, 2019
I think the second option is better, as it's BC from a config standpoint. It might break for invalid configs but that's probably a good thing..
Could you post the correct configuration though? Because I am still not sure what the error was in your config. I see a type:stream which makes no sense to me as you have a
handler: groupedas well which can not be present on stream handlers as they don't accept nested handlers.DonCallisto commentedon Oct 7, 2019
Yeah, that's true, it was a bad configuration (no sense one indeed) so the right one is
If you agree with the second option and no other ideas arise, I'll work on this on the next days.
DonCallisto commentedon Oct 19, 2019
I'm working on this issue, trying to validate the configuration directly in
Configurationfile.One thing that, at the moment, is preventing me to complete the work is the default values of some nodes as, in
validationphase, I get them from the configuration but I need to throw an exception only if user explictly defined them (in case that those values are simply "discharged" by the handler type).Any ideas?
DonCallisto commentedon Oct 19, 2019
Maybe there’s the chance to assign a default value only under certain circumstances (ie based on handler type?). I need to check.
DonCallisto commentedon Oct 20, 2019
This is a WIP but I've found, maybe, how to accomplish the combination of not having a default value (when that value is not intended to be used from handler type), letting the user use that attribute and throwing and exception if the attribute should not be used in the handler type.
Check it out https://github.com/symfony/monolog-bundle/compare/master...DonCallisto:config_validation?expand=1#diff-850942b3ba24ab03a40aaa81b6152852
WDYT?
DonCallisto commentedon Oct 22, 2019
Umh, diggin' my toes deeper I've found that
beforeNormalizationprobably is not the right place to add the default values (in legit cases indeed the default value is not added).I'll try to look deeper in how the
Configurationworks, in the meantime if someone has any ideas is welcome to share.Thanks.
DonCallisto commentedon Oct 27, 2019
Ok I finally found how to proceed. I should use
beforeNormalizationon the wholeArrayNodein order to have the values defined by the user.I'll work on this in the next days and I should be able to complete the task within the next week, hopefully.
DonCallisto commentedon Oct 28, 2019
I'm almost there.
Just wanted to share what I've done so far https://github.com/symfony/monolog-bundle/compare/master...DonCallisto:config_validation?expand=1
I'll finish in the next days.
WDYT? Is it good?
uncaught commentedon Oct 30, 2022
You shouldn't validate config values at compile time. This breaks working with environment variables at runtime. Nobody should have to rebuild the container everytime the environment changes. That's often not even possible in a dockerized world.
This bundle already blocks several parameters from being used as real runtime environment variables, please don't make it more.
Even changing a handler type could make sense as environment variable (production container on AWS using a different handler than a production container on a simple webserver, etc.). But I understand that would lead to different built services with the current architecture.
If you need validation, you could make it a command, which can be run anytime you want and then evaluate the values at that time correctly.