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

fixes #342 add rfc option for syslogudp handler #343

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

gwinn
Copy link

@gwinn gwinn commented Mar 4, 2020

No description provided.

@@ -477,6 +478,7 @@ public function getConfigTreeBuilder()
->scalarNode('title')->defaultNull()->end() // pushover
->scalarNode('host')->defaultNull()->end() // syslogudp & hipchat
->scalarNode('port')->defaultValue(514)->end() // syslogudp
->scalarNode('rfc')->defaultNull()->end() // syslogudp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be an integerNode as SyslogUdpHandler constructor requires an int

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also please add description and potential values like \Monolog\Handler\SyslogUdpHandler::RFC3164

@@ -157,6 +157,7 @@
* - [level]: level name or int value, defaults to DEBUG
* - [bubble]: bool, defaults to true
* - [ident]: string, defaults to
* - [rfc]: string, defaults to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaults to ...? it does not have a default. also it's an integer

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has, https://github.com/Seldaek/monolog/blob/master/src/Monolog/Handler/SyslogUdpHandler.php#L47

But yes, it is not correct to describe it as string

@gwinn gwinn requested a review from Tobion March 9, 2020 16:07
@gwinn
Copy link
Author

gwinn commented Jul 3, 2020

Can you review latest changes?

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
@iyzoer
Copy link

iyzoer commented Jan 19, 2021

Hi! When can we expect a merge? This very important for me. It seems like nothing else prevents the merge.

@HontoNoRoger
Copy link

Also highly requested from my side, need it to select RFC5424e (time with miliseconds) for SyslogUDP logging.

Copy link
Contributor

@dbrumann dbrumann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have a test case in Tests/DependencyInjection/ConfigurationTest.php to see if the default is used when rfc is not set in config and another case for when an invalid number is picked.

@@ -486,6 +488,10 @@ public function getConfigTreeBuilder()
->scalarNode('title')->defaultNull()->end() // pushover
->scalarNode('host')->defaultNull()->end() // syslogudp & hipchat
->scalarNode('port')->defaultValue(514)->end() // syslogudp
->enumNode('rfc')
->values([SyslogUdpHandler::RFC5424, SyslogUdpHandler::RFC3164])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about RFC5424e?

If we add this, we probably need to be careful, not to rely on the constant as UdpHandler in Monolog 1 does not support it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why i'm not sure about this RFC at this moment.
Should it be added at UdpHandler in Monolog 1? Or should it be added in some other place and imported alongside with SuslogUdpHandler for Monolog v1.x?

Copy link
Contributor

@dbrumann dbrumann Jan 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how actively maintained v1 is, i.e. whether new features are accepted and if this is considered a new feature.

Both UdpHandler v1 and v2 will just ignore invalid values as far as I can tell. I think we can just allow users to put in whatever int they want, maybe add a comment for where to find the proper values (e.g. "see UdpHandler RFC-constants") and not bother with the constants at all. Not sure what others think about this, though.

edit: I just looked at the handlers again and it seems specifying an invalid int will at least trigger a notice about an illegal array offset, but I think that could be easily fixed using an array_key_exists()-check with a fallback.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, i think we should leave this story as is at this moment, we can add support for RFC5424e into another PR to speed up the acceptance of the current request

@gwinn gwinn force-pushed the master branch 2 times, most recently from df1782a to c4393a5 Compare February 1, 2021 17:02
@gwinn
Copy link
Author

gwinn commented Feb 1, 2021

It would be nice to have a test case in Tests/DependencyInjection/ConfigurationTest.php to see if the default is used when rfc is not set in config and another case for when an invalid number is picked.

Done

DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
DependencyInjection/MonologExtension.php Show resolved Hide resolved
@gwinn gwinn requested a review from jderusse March 25, 2021 09:54
@gwinn
Copy link
Author

gwinn commented Mar 25, 2021

Can I expect a review soon? It seems to me important and necessary to contribute to open source projects, especially if the proposed changes are needed not only by one developer, but, frankly, it is a little confusing how quickly the library development team considers proposals, this PR was created a year ago

@@ -486,6 +488,10 @@ public function getConfigTreeBuilder()
->scalarNode('title')->defaultNull()->end() // pushover
->scalarNode('host')->defaultNull()->end() // syslogudp & hipchat
->scalarNode('port')->defaultValue(514)->end() // syslogudp
->enumNode('rfc')
->values([SyslogUdpHandler::RFC5424, SyslogUdpHandler::RFC3164])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using raw value (cf snippet bellow) would allow people to use the RFC, without triggering exception for composer 1

Suggested change
->values([SyslogUdpHandler::RFC5424, SyslogUdpHandler::RFC3164])
->values([SyslogUdpHandler::RFC5424, SyslogUdpHandler::RFC3164, 2 /* SyslogUdpHandler::RFC5424e */])

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#343 (comment) here @Tobion suggest to not to use plain values, which point of view should I adhere to

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's always better to use constant, that's why @Tobion suggested to not use plain value when a constant exists.

In this case (and only for the value 2), we can not use constant, as the constant does not exist in monolog 1.
But we should not prevent people that use monolog 2 to use this value.
As a result, you should either remove this ENUM constraint and let people enter whatever they want, or allows this rfc by using a plain value.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i'll change it to scalarNode with default value

DependencyInjection/MonologExtension.php Show resolved Hide resolved
Copy link
Member

@jderusse jderusse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A last comment for my side ;-).

Don't forget to add an entry in the ChangeLog 🙏

DependencyInjection/MonologExtension.php Outdated Show resolved Hide resolved
@gwinn
Copy link
Author

gwinn commented Mar 31, 2021

A last comment for my side ;-).

Don't forget to add an entry in the ChangeLog pray

done )

@@ -408,7 +410,7 @@ public function getConfigTreeBuilder()
->booleanNode('use_locking')->defaultFalse()->end() // stream and rotating
->scalarNode('filename_format')->defaultValue('{filename}-{date}')->end() //rotating
->scalarNode('date_format')->defaultValue('Y-m-d')->end() //rotating
->scalarNode('ident')->defaultFalse()->end() // syslog and syslogudp
->scalarNode('ident')->defaultValue('php')->end() // syslog and syslogudp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a bc break. The https://github.com/Seldaek/monolog/blob/main/src/Monolog/Handler/SyslogHandler.php does not have 'php' as default value. So it used an empty string (casted from false) before.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but @Tobion is right here, this configuration is also used by both Syslog and SyslogUdp.
The change should keep compatibility with the 2 handlers.

Could you also please add a test to cover this case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants