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

Fix code example #800

Closed
wants to merge 1 commit into from
Closed

Fix code example #800

wants to merge 1 commit into from

Conversation

Horttcore
Copy link

No description provided.

Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

This is correct, thanks for spotting this! Registering the handler though is not enough, thanks to #762 (and #788 in the future); so, to make the example complete, you should add the listener registration.

@ste93cry
Copy link
Collaborator

ste93cry commented Apr 3, 2019

Since I'm evaluating in #788 to stop registering implicitly the error handler in the ErrorHandler::add*Listener methods (and so make them non-static), to be future-proof I would change the example to something similar to

$errorHandler = ErrorHandler::registerOnce();
$errorHandler->addErrorListener(...);
$errorHandler->addFatalErrorListener(...);
$errorHandler->addExceptionListener(...);

@ste93cry ste93cry added this to the 2.0 milestone Apr 4, 2019
@ste93cry
Copy link
Collaborator

ste93cry commented May 5, 2019

The PR I was talking about has been merged, but the changes will be available only from version 2.1. Since the UPGRADE-2.0.md is targeting of course the 2.0 version it seems to me that the changes here are fine as they are except for the fact that as said by @Jean85 we should document that at least a listener should be added to report the errors. @Horttcore are you willing to address the requested changes so that we can merge this PR?

@ste93cry ste93cry removed the Type: Bug label Oct 4, 2019
@stayallive
Copy link
Collaborator

I am closing this PR because of inactivity, I even believe this problem was eventually addressed.

@stayallive stayallive closed this Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants