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

Silence E_WARNING "exceptions" #12517

Merged
merged 3 commits into from
Feb 15, 2023
Merged

Conversation

uberbrady
Copy link
Collaborator

@uberbrady uberbrady commented Feb 15, 2023

This is really only going to affect people who use Rollbar - which is probably mostly just us.

The background for this issue is complicated, so I'll tl;dr it here and say - "This fix makes it so that error-suppressed things like @$array['nonexistent_element'] will no longer send to Rollbar when in Production-mode".

I tested that with and without the @ in front of it. And without the @, it still alerts Rollbar (and, also, fails to render the page, showing an error screen instead). With the '@' it prints a message to the Log. Full-fledged Exceptions continue to alert Rollbar normally.

One concern I have - and I really have no idea how to 'fix' this concern - is that things like Parse errors are going to show weird error messages like "class \Log not found" which is going to look funny. Maybe I can find a way to detect parse errors in Pure PHP and do an early-return of true so "normal" PHP error handling catches it all? Happy to discuss it, because it definitely will throw us for a loop at some point. - RESOLVED!

Long Explanation

There's a lot of complicated stuff going on here. Regular 'exceptions' work just fine. Really. They do pretty much what you want them to. However, "old-school" PHP "errors" and such are not. Modern frameworks like Laravel will typically patch in a set_error_handler() callback that will turn those errors and warnings into Exceptions, which will filter their way up the stack. Furthermore, Rollbar itself does the same thing - in order to report them to Rollbar. So I can't really tell which error handler is messing up which, here - but it's very confusing.

The best solution I could come up with is to figure out how to use Rollbar's check_ignore config option, and find things that were originally E_WARNING and filter those out - only when in Production, though.

@what-the-diff
Copy link

what-the-diff bot commented Feb 15, 2023

  • Added a check_ignore function to the Rollbar handler.
  • The function checks if we are in production mode and that the error is an E_WARNING, then logs it as info (instead of warning) and returns true so rollbar ignores it.

@snipe
Copy link
Owner

snipe commented Feb 15, 2023

This looks great, @uberbrady - happy to discuss your concerns with you tomorrow!

@uberbrady
Copy link
Collaborator Author

Okay, I have not only handled my concern, I have some bonus things that I've managed to pull off!!!

So, we have typically had problems where, if an error happens "early up in the stack" - you would get a shitty error message in plain, unformatted HTML saying "Class Log not found."

With the latest push to this PR, that now goes away - you can even do edits to the config files with syntax errors, and you'll get a reasonable-looking error message. You can even mess around in the Middleware, and you'll keep getting those reasonable-looking error messages.

This is going to be a HUGE boost to us when we're mucking around in the guts of stuff throughout the project. (Which, admittedly, we don't do often - but when we do, this will help a ton).

I feel really good about this now, I think it's ready to go!

@snipe
Copy link
Owner

snipe commented Feb 15, 2023

This looks excellent - thank you Brady!

@snipe snipe merged commit 7980e2a into snipe:develop Feb 15, 2023
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.

None yet

2 participants