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

Catch and log exceptions thrown when actions can't be created, e.g. under a corrupt database schema #1000

Merged
merged 11 commits into from
Oct 24, 2023

Conversation

lsinger
Copy link
Contributor

@lsinger lsinger commented Oct 9, 2023

This PR attempts to address parts of #973 by catching and logging the exceptions that are thrown when actions can't be created. If, e.g., a database schema migration did not complete properly, scheduling an action has been able to render a site inaccessible because exceptions just bubbled up to the user.

This PR explicitly does not solve completing or fixing incomplete schema migrations -- that's a different task. The first step implemented here is to lessen the impact of such issues.

To test this manually, you could take a working site with AS running fine and then drop the priority column. Without this PR the exceptions thrown when trying to schedule actions bubble up to the user, with this PR applied they're merely logged. AS isn't functional then but at least we limit the damage.

Alternatively apply the commits from this branch sequentially and run the test suite in between:

  • ff2c680 makes the test suite fail from the uncaught exceptions
  • 25a43f2 catches the exceptions and makes the test suite pass
  • e1bd2e9 adds some requirements around how we log the exceptions to the tests and makes the test suite fail again
  • e5c427f adds the exception message to the log entry and thus makes the test suite pass again
  • (27bf749 and b0864e0 are not covered by tests)

Changelog

Fix - Reduce impact when the database schema cannot be modified following an update.

@lsinger lsinger requested a review from a team October 9, 2023 13:40
@lsinger lsinger self-assigned this Oct 9, 2023
@lsinger lsinger requested review from vedanshujain, a team and barryhughes and removed request for a team and vedanshujain October 9, 2023 13:40
functions.php Outdated Show resolved Hide resolved
functions.php Outdated Show resolved Hide resolved
functions.php Outdated Show resolved Hide resolved
@lsinger lsinger force-pushed the fix/invalid-schema-exceptions-not-caught branch from 10cfe70 to 0933485 Compare October 24, 2023 14:53
@lsinger lsinger requested a review from barryhughes October 24, 2023 15:11
Copy link
Member

@barryhughes barryhughes left a comment

Choose a reason for hiding this comment

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

This is great, thank you.

  • Confirmed a (potentially site-breaking) exception occurs without this change (when the priority column is missing).
  • Confirmed an exception does not occur when this branch is checkout out, and that appropriate error messages are logged.

@barryhughes barryhughes added this to the 3.7.0 milestone Oct 24, 2023
@barryhughes barryhughes merged commit be4a6f4 into trunk Oct 24, 2023
44 checks passed
@barryhughes barryhughes deleted the fix/invalid-schema-exceptions-not-caught branch October 24, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants