-
-
Notifications
You must be signed in to change notification settings - Fork 240
Make each handler type have its own extension class #560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 3.x
Are you sure you want to change the base?
Conversation
stof
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want external bundles to extend the MonologBundle configuration class. This creates extra complexity:
- depending on where the Configuration class is accessed, the extensions might be registered or no (we have that issue in SecurityBundle, which is the only bundle I'm aware of doing such thing
- it makes it harder to ensure BC if we don't control the config tree
- most of our handlers supported in core don't group their config settings in a dedicated sub-node in the config tree, which means they cannot be migrated to this new system (at least not without a complex BC layer to deprecate the existing nodes, which will be very hard to implement properly given that some of our config settings are used by multiple handlers).
We already have the service type to allow configuring handlers that are not supported in core.
| * bubble: bool, | ||
| * formatter: string|null, | ||
| * } $config Generic options | ||
| * @param array{} $handler Specific handler options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is wrong. array{} is the shape representing an empty array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is on purpose. By default this is an empty array. You have to specify another array shape in subclasses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not the proper way to describe such API. We would need to use a generic type with THandlerConfig of array<string, mixed>, with child classes describing their array shape as the template type.
|
|
Could this also be used to pass the handler's FQCN as 'handlers' => [
'foobar' => [
'type' => StreamHandler::class |
This is a partial PR to gather feedbacks.