Skip to content
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

Add constructor injection for SocketHandler and its children #1600

Merged
merged 2 commits into from
Oct 13, 2021

Conversation

boesing
Copy link
Contributor

@boesing boesing commented Oct 1, 2021

Hey there,

I'd like to propose a PoC to allow constructor injection for the SocketHandler and its children.
Is that something you want to support within monolog?

If not, I'd be fine with closing this. I can wrap around this and implement this as a feature in a dedicated abstraction layer tho.

Thanks in advance,
Max

@Seldaek
Copy link
Owner

Seldaek commented Oct 1, 2021

I guess why not, although it makes the signatures much longer and complex to grasp, I do see the value/ease of use too.

Can you please update the phpdoc to add the 5 new params in SocketHandler tho? No need to duplicate the descriptions everywhere IMO, as these get inherited, but it'd be nice to have details on the parent class.

@Seldaek Seldaek added this to the 2.x milestone Oct 1, 2021
@Seldaek Seldaek added the Feature label Oct 1, 2021
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing boesing force-pushed the feature/socket-constructor-injection branch from 09bda40 to 8ca1cdb Compare October 5, 2021 09:25
@boesing
Copy link
Contributor Author

boesing commented Oct 5, 2021

I've added the new arguments to phpdoc 🤙🏼

@Seldaek Seldaek merged commit 4a11cad into Seldaek:main Oct 13, 2021
@Seldaek
Copy link
Owner

Seldaek commented Oct 13, 2021

Thanks

@boesing boesing deleted the feature/socket-constructor-injection branch October 13, 2021 21:25
@lyrixx lyrixx mentioned this pull request Aug 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants