search

woocommerce / action-scheduler

A scalable, traceable job queue for background processing large queues of tasks in WordPress. Specifically designed for distribution in WordPress plugins (and themes) - no server access required.
https://actionscheduler.org
GNU General Public License v3.0
641 stars 117 forks source link

More descriptive exception message #994

Open KarlisJ opened 1 year ago

KarlisJ commented 1 year ago

We observed a case of getting an error logged (6929293-zd-a8c)

scheduled action 19105 (subscription payment) failed to finish processing due to the following exception: Call to a member function set() on null

which doesn't help debug what is causing the issue.

Changing $e->getMessage() to $e->getTraceAsString() in this line: https://github.com/woocommerce/action-scheduler/blob/0bed905d55cc043ccb6175e66e0d4977a81c173a/classes/abstracts/ActionScheduler_Abstract_QueueRunner.php#L94 gave more descriptive message indicating to the conflicting plugin.

I'm not sure if that change is the best approach but in general the existing reporting does obscure crucial debugging information.

james-allan commented 6 months ago

Changing $e->getMessage() to $e->getTraceAsString() in this line: https://github.com/woocommerce/action-scheduler/blob/0bed905d55cc043ccb6175e66e0d4977a81c173a/classes/abstracts/ActionScheduler_Abstract_QueueRunner.php#L94

I looked into this briefly while submitting https://github.com/woocommerce/action-scheduler/pull/1057 and wanted to add my two cents.

I don't think the suggested approach of creating the exception with the trace as the message is the right approach here. When the exception is created it is passed the previous exception, and so the subsequent code is capable of pulling out the exception message and trace from that previous exception as necessary.

IMO the approach should be either:

  1. Update the handle_action_error() function which logs the message to handle pulling the line and trace information from the previous exception. ie $e->getPrevious()->getTraceAsString()
  2. Update the call to $this->handle_action_error( $action_id, $e, $context, $valid_action ); (here) to pass the previous exception rather than $e. eg $this->handle_action_error( $action_id, $e->getPrevious(), $context, $valid_action );

I'm not familiar with this section of code to understand the nested try catch to make a judgment on the right way forward here.

barryhughes commented 2 months ago

Sorry for the late response ...

I'm not familiar with this section of code to understand the nested try catch to make a judgment on the right way forward here.

The nested try/catch was mostly to support PHP 5.6 (no support for throwables) alongside PHP 7.0+ (added a concept of throwable errors). We no longer support 5.6, so we may now be able to drastically simplify this and remove the nesting.