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

More descriptive exception message #994

Open
KarlisJ opened this issue Sep 21, 2023 · 2 comments
Open

More descriptive exception message #994

KarlisJ opened this issue Sep 21, 2023 · 2 comments
Labels
priority: normal The issue/PR is normal priority—not many people are affected or there’s a workaround, etc. type: enhancement The issue is a request for an enhancement.

Comments

@KarlisJ
Copy link

KarlisJ commented Sep 21, 2023

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.

@jorgeatorres jorgeatorres added type: enhancement The issue is a request for an enhancement. priority: normal The issue/PR is normal priority—not many people are affected or there’s a workaround, etc. labels Sep 22, 2023
@james-allan
Copy link
Contributor

Changing $e->getMessage() to $e->getTraceAsString() in this line:

throw new Exception( $e->getMessage(), $e->getCode(), $e->getPrevious() );

I looked into this briefly while submitting #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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: normal The issue/PR is normal priority—not many people are affected or there’s a workaround, etc. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

No branches or pull requests

4 participants