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

Added errorhandler to convert errors to exceptions #30

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

harmenjanssen
Copy link
Member

This is a followup to the discussion in issue #27.
All PHP errors that can be handled with set_error_handler are
converted to Garp_Exception_RuntimeException instances of the correct
severity.

This makes sure not even a notice slips through.

‼️ Let's not merge this immediately by the way, but pick a project to run this in for a while and see what turns up in production.

@aapit
Copy link
Contributor

aapit commented Apr 18, 2017

LGTM!
It's hard to spot any flaky syntax or whatever like that at this point, but this is a very good idea in general.

@harmenjanssen harmenjanssen changed the base branch from develop to master May 22, 2017 08:18
@harmenjanssen harmenjanssen reopened this May 22, 2017
@aapit
Copy link
Contributor

aapit commented May 22, 2017

Are we running this one somewhere yet?

@harmenjanssen
Copy link
Member Author

No, we're not. Hmm.
I have tried it in a Twig project a while ago and got hammered with deprecation notices from the Twig library.

Anyways, that's not to say we shouldn't do it, but it'll need some attention probably to keep errors down a bit.

@aapit
Copy link
Contributor

aapit commented May 24, 2017

Can you book some time for this? No rush, but it would be good to have it planned.

@harmenjanssen
Copy link
Member Author

harmenjanssen commented May 24, 2017 via email

@aapit aapit assigned harmenjanssen and unassigned HammenWS and aapit Jun 15, 2017
@harmenjanssen harmenjanssen force-pushed the feature/error-reporting branch 2 times, most recently from 4464f30 to fbc8c11 Compare July 4, 2017 07:07
This is a followup to the discussion in issue #27.
All PHP errors that can be handled with `set_error_handler` are
converted to `Garp_Exception_RuntimeException` instances of the correct
severity.

This makes sure not even a notice slips through.
@harmenjanssen harmenjanssen force-pushed the feature/error-reporting branch from fbc8c11 to fcd1f81 Compare July 4, 2017 07:55
@harmenjanssen
Copy link
Member Author

I'm using this branch in Subsidieportaal. Let's see what happens.
If no notable errors happen in a month or so I'll merge this to master, bumping a major version (I don't want this to automatically propagate to every project).

@aapit
Copy link
Contributor

aapit commented Jul 4, 2017 via email

@harmenjanssen
Copy link
Member Author

Hmm, conclusions so far: it's a bit annoying to face warnings like "open basedir restriction in effect", when just asking if file_exists. They could've just chosen to return FALSE, and surely everything would just keep on working fine.

Anyways.

The current implementation has one fairly large disadvantage: because the exception comes from the error handler, they're sort of on a different track. Meaning, if I would wrap my entire app from start to finish in a try { ... } catch, I still would not be able to catch that exception. Unfortunately, that means Sentry won't pick up on it. So it needs further investigation. Maybe we could use set_exception_handler?

I've taken it out of Subsidieportaal for now, because Zend's cache helper does the aforementioned file_exists check. I don't know why yet, but pages using that helper would just bork completely.

@aapit
Copy link
Contributor

aapit commented Jul 10, 2017 via email

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.

3 participants