(Optional) improvements for handling a subset of deserialization errors#1345
Closed
barryhughes wants to merge 8 commits into
Closed
(Optional) improvements for handling a subset of deserialization errors#1345barryhughes wants to merge 8 commits into
barryhughes wants to merge 8 commits into
Conversation
A structurally-valid schedule that merely nests a class Action Scheduler does not recognize (an un-allow-listed, non-schedule class) is not evidently dangerous -- just unknown. Returning false for it, as the hardening does, makes the stores treat it as corrupt and the queue runner silently cancel the action, losing work a site operator might still need. Introduce ActionScheduler_UnrecognizedSchedule (a NullSchedule placeholder that records the unrecognized class names) and return it from the deserializer for the recoverable ($shadow_eligible) case, so the action stays a valid, displayable schedule. Structural rejections -- a top-level non-schedule, an unwalkable graph, or a throwing __wakeup -- have no legitimate form and remain corrupt (false). This is the foundation for routing such actions to a failed (reviewable, forced- re-runnable) state instead of cancelling them. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e-run The queue runner now marks an action failed -- for operator review -- when its schedule resolves to an ActionScheduler_UnrecognizedSchedule, instead of silently cancelling it as corrupt. We do not auto-run it, because its intended timing is unknown. Site operators who cannot change code still need to keep work flowing, so add force_run_action() (backed by a new $force flag on process_action()) which executes such an action's callback despite the unreadable schedule and completes it. A failed action would otherwise fetch as a non-executable FinishedAction, so for the duration of a forced run it is mapped to an executable action class. A new action_scheduler_unrecognized_schedule_action hook fires on the failure. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d schedules The admin "Run" row action now force-runs a failed action (previously a no-op, since a failed action is not pending), so an operator can retry a failed action or flush one whose schedule could not be recognized -- without needing to change code. Pending actions keep the normal run path. The Scheduled Date column renders an unrecognized schedule as "Unrecognized schedule" instead of falling through to the NullSchedule "async" label. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the queue runner fails an action because its schedule references an unrecognized class, record a (non-autoloaded) flag. On admin pages, show a notice to users who can manage options, linking to the failed actions screen and explaining they can force-run an action that is safe. The notice renders until the operator dismisses it (a nonce-guarded dismissal clears the flag); a later failure sets it again. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review follow-up. The list table routes every failed action to force_run_action(), including one that retains a valid recurring schedule. process_action() would then reach the tail reschedule and schedule another instance -- but the series already advanced when the action first ran, so this created a duplicate. Suppress the reschedule on any forced run (which also makes the code match force_run_action()'s documented contract). Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Translators should not need to understand HTML. Restructure the notice so every
translatable string is plain text: the sentence carries only numbered
placeholders, and the anchor text ("failed", "Dismiss") and label are separate
plain strings. Markup is assembled in PHP and interpolated via sprintf, with each
dynamic part escaped (esc_url/esc_html__).
esc_html__() on the sentence also prevents a rogue translation from injecting
markup, so wp_kses() -- which would instead allow whitelisted tags through -- is
no longer needed.
Refs #1318
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Splitting the anchor text into separate __() calls made the sentence a set of fragments, which translates poorly (word order, agreement and declension vary by language, and a lone "failed" has no context). Keep the entire sentence as a single translatable string and pull only the HTML tags out as opening/closing placeholder pairs, so the linked phrases stay in-sentence and can move as a unit. Escaping is unchanged: esc_html__() on the sentence still neutralizes rogue- translation markup, and each opening tag carries an esc_url()'d URL, so no wp_kses. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Document the new $force parameter on process_action(); add the missing short description to ActionScheduler_UnrecognizedSchedule::__construct(); expand the single-line test closures and the associative array to satisfy the WordPress coding standard; and annotate the deliberate $_GET cleanup in a test. Refs #1318 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Closing. This is currently incomplete, and it's not really clear if we are going in the right direction: what we really need to do is have a little more discussion about when and how we notify site operators of problems. The specific issue we were concerned with here (cancellation due to unsafe scheduling data) is expected to occur only very rarely, and many sites will never encounter it at all. This begs the question, why go to special measures for this scenario when other action failures receive no such additional support (yet, something like a series of failing payment actions could be a serious matter for a merchant)? A partial answer is that by merging #1344 we would be introducing a new reason for automatic action cancellation that did not previous exist, but the question still stands. Last note: some (at least partly related) prior art exists in the form of past-due notices and the past-due action view. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.