-
Notifications
You must be signed in to change notification settings - Fork 122
Throw an ErrorException rather than a standard exception when an error is caught during action processing #1057
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
base: trunk
Are you sure you want to change the base?
Conversation
function ( $type, $message ) { | ||
throw new Exception( $message ); | ||
function ( $type, $message, $file, $line ) { | ||
throw new ErrorException( $message, 0, $type, $file, $line ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@james-allan I'm not sure this is enough (or at least, may not work as expected with recent PHP runtimes). When I test:
action_scheduler_begin_execute
is fired from within a try {} block, and the test snippet then triggers an error.- We enter this error handler, and throw the
ErrorException
. - We now return to the matching catch {} block which converts the throwable (in this case, the
ErrorException
) into a regular exception.
So, in my own local testing, I do not see the same result you document in the testing instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, I tested with PHP 8.3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though it goes a little beyond the problem you are immediately trying to solve, I'm wondering if we can greatly simplify things (specifically, remove the custom error handler and the nested try structure), now that we no longer need to support PHP 5.6 🤔
Hey @james-allan thanks for your contribution! |
I think(?) this PR fixes an issue I've been running into more frequently, which is Actions that fail for a really basic reason but I have no visibility into which file/line number. The AS logs for failed actions show the following, with just the The error shows no directory, no file, no line number. And it's stock/vanilla AS too; I haven't altered anything about it. It's been a real bear replicating the error in a non-Action environment so that the error actually prints out, rather than bubbles up through the AS system and just shows the message with nothing else. |
Fixes #1052
Description
In the
ActionScheduler_Abstract_QueueRunner::process_action()
function AS sets an error handler and anyE_USER_ERROR
orE_RECOVERABLE_ERROR
error that occurs while processing the action is caught and thrown as an exception instead.This is super handy for WooCommerce Subscriptions because AS will trigger the
action_scheduler_failed_execution
action which passes callbacks the exception that was caught.WooCommerce Subscriptions uses this hook to log data about why our subscription-related scheduled actions have failed, however something I noticed today is that because AS is creating a new exception, if you use the
$exception->getFile()
$exception->getLine()
helpers to point to where the exception occurred, it will point to AS, not where the error was actually thrown.This is fixed by throwing an
ErrorException
which enables specifying the file and line where the error occurred.Testing instructions
trunk
you'll notice the file and line points to AStrunk