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

Indicate when message is not processed by the monolog handler, to disable bubbling #1189

Closed
EvgeniiR opened this issue Feb 21, 2021 · 6 comments

Comments

@EvgeniiR
Copy link

EvgeniiR commented Feb 21, 2021

Hello!
Monolog\Handler\HandlerInterface::handle allows returning false to indicate that message is not handled for some reason, so it'll be passed to next registered monolog handlers, even when $bubble == true.

I think it would be great for sentry monolog handler to overwrite this method to return false when some error happened during processing

@ste93cry
Copy link
Collaborator

By default, the $bubble argument of the constructor is set to true. This means that since the base implementation class does false === $this->bubble, it should return false every time. Are you using the handler with $bubble = false instead?

@EvgeniiR
Copy link
Author

@ste93cry I'm using $bubble = false to prevent future processing after event is sent to Sentry, but sometimes my on-premise instance of Sentry may be unavailable, so I want to fallback to another error monitoring system

@ste93cry
Copy link
Collaborator

Wouldn't changing the current behavior go against the purpose of the $bubble argument?

Whether the messages that are handled can bubble up the stack or not

https://github.com/Seldaek/monolog/blob/c1971982c3b792e488726468046b1ef977934c6a/src/Monolog/Handler/AbstractHandler.php#L29

true means that this handler allows bubbling. false means that bubbling is not permitted.

https://github.com/Seldaek/monolog/blob/c1971982c3b792e488726468046b1ef977934c6a/src/Monolog/Handler/AbstractHandler.php#L71

Basically, you're saying that in case an error occurs while sending the event, you would like the message to be handled by other handlers, regardless of whether this is permitted or not according to $bubble. Am I right?

@EvgeniiR
Copy link
Author

EvgeniiR commented Mar 12, 2021

Basically, you're saying that in case an error occurs while sending the event, you would like the message to be handled by other handlers, regardless of whether this is permitted or not according to $bubble. Am I right?

Yep!

Whether the messages that are handled can bubble up the stack or not

I'm considering message as "not handled" when error occurred while sending.

Also, PHPdoc here says:

return bool true means that this handler handled the record, and that bubbling is not permitted. false means the record was either not processed or that this handler allows bubbling.

@ste93cry
Copy link
Collaborator

ste93cry commented Mar 12, 2021

I'm considering message as "not handled" when error occurred while sending.

The second statement though is more general in the wording instead, therefore open to a possible different interpretation. I had a look at the built-in handlers of Monolog and they all avoid bubbling if $bubble = false, therefore our handler is in effect respecting the general expected behaviour. Plus, we're implementing the write() method whose return type is void, definitely preventing any way to return something to the caller (handle()) and consequently reinforcing my idea that this is done by-design. I definitely see the usefulness of the request, but I suggest to open an issue in the Monolog repo to understand what is the general expectation before making any change here and diverging the behaviour from all the other handlers

@github-actions
Copy link

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

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

No branches or pull requests

3 participants