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

add exception handler for invalid schedule in make_action_from_post() #292

Merged
merged 4 commits into from
Apr 18, 2019

Conversation

rrennick
Copy link
Contributor

This PR adds an exception handler for the WP post store retrieving the schedule from the post content. It's possible for the post content to be json_encoded and not an array.

@mikejolley Can you take a look at this as well?

@rrennick rrennick requested a review from JPry April 18, 2019 15:40
@rrennick rrennick added type: bug The issue is a confirmed bug. priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. Status: Needs Review labels Apr 18, 2019
@mikejolley
Copy link
Member

@rrennick this looks good and looks like it will mitigate the issue. Just to point out though @JPry was talking about logging these somewhere, and noted that there are try/catches in place in other places which call this method. However, I found a few which don't.

So the choice seems to be try/catch here, or try/catch higher up when fetching these jobs.

@rrennick
Copy link
Contributor Author

Just to point out though @JPry was talking about logging these somewhere

I think it would be good to log it in the scheduled action log in addition to adding it to an error log. In that case, it should be done above the data store so it's logged regardless of the data store being used.

Copy link
Contributor

@JPry JPry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rrennick This does indeed solve the problem of catching the exception, but it leaves this problem failing silently. I think we should add an action inside the catch block that ActionScheduler_FatalErrorMonitor (or other code) can tie into.

@rrennick
Copy link
Contributor Author

@JPry Thanks, I added the hook and an action to the base logger class to log to the action that it failed to fetch.

Copy link
Contributor

@JPry JPry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high The issue/PR is high priority—it affects lots of customers substantially, but not critically. type: bug The issue is a confirmed bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants