Plugin Directory: render cron job argument values as text in the logs metabox - #858
Plugin Directory: render cron job argument values as text in the logs metabox#858obenland wants to merge 2 commits into
Conversation
… metabox The Cron Job Logs metabox builds each job's task description from the job's own arguments. `tags_touched` carries SVN tag folder names and the tag named by a release-confirmation request, so its values are free-form strings that should display literally rather than as part of the surrounding markup. Adds coverage for the metabox, with a small Cavalcade job stub so the rendering path is reachable from the test suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe cron logs metabox now HTML-escapes dynamic job argument values. PHPUnit tests cover escaped values, valid arguments, empty job output, and isolated Cavalcade job fixtures. ChangesCron log escaping
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change renders cron job argument values as literal text in the logs metabox, with no actionable merge-blocking risk remaining after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Updates the Plugin Directory’s Cron Job Logs metabox to safely render cron job argument values as literal text (not markup), ensuring free-form values like tags_touched can’t inject HTML into the admin UI.
Changes:
- Escape rendered cron-job argument values in
Cron_Logs::display()usingesc_html(). - Add a PHPUnit test suite that renders the metabox output and asserts argument values remain escaped.
- Introduce a minimal Cavalcade
Jobmodel fixture for tests to makeManager::get_plugin_cron_jobs()reachable.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
wordpress.org/public_html/wp-content/plugins/plugin-directory/admin/metabox/class-cron-logs.php |
Escapes cron job argument values before embedding them in the metabox HTML. |
wordpress.org/public_html/wp-content/plugins/plugin-directory/tests/Cron_Logs_Metabox_Test.php |
Adds metabox rendering tests covering HTML-in-args escaping and empty state. |
wordpress.org/public_html/wp-content/plugins/plugin-directory/tests/fixtures/cavalcade/class-job.php |
Adds a stub Cavalcade Job class to supply deterministic jobs to the metabox during tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $GLOBALS['post'] = $plugin; | ||
|
|
||
| /* | ||
| * The metabox always asks for logs, and the Cavalcade log table doesn't exist | ||
| * in the test install. The failed lookup is the same no-logs case as a job that | ||
| * hasn't run yet; suppress the error so it doesn't reach the output buffer. | ||
| */ | ||
| $GLOBALS['wpdb']->suppress_errors( true ); | ||
| } | ||
|
|
||
| /** | ||
| * Reset the globals the metabox and its job source read. | ||
| */ | ||
| protected function tearDown(): void { | ||
| \HM\Cavalcade\Plugin\Job::$jobs = array(); | ||
|
|
||
| $GLOBALS['wpdb']->suppress_errors( false ); | ||
| unset( $GLOBALS['post'] ); | ||
|
|
||
| parent::tearDown(); | ||
| } |
There was a problem hiding this comment.
Good catch — restored in ebb4599. setUp() now records whether a global post existed and what suppress_errors() returned, and tearDown() puts both back instead of clearing them. This suite has no per-test transactions by design, so leaked globals are exactly the failure mode it can't absorb.
No other test in the plugin touches either global today, so this isn't fixing a live break — but the restore is a variable each, and it keeps the next test that does touch them from inheriting this one's state.
Restore the previous global post and wpdb error-suppression setting in `tearDown()` rather than clearing them outright, and scope the log-row assertion to the row it names — the task description above it carries the same value, so a page-wide assertion couldn't tell the two apart. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Cron Job Logs metabox builds each job's task description from the job's own arguments.
tags_touchedcarries SVN tag folder names and the tag named by a release-confirmation request, so its values are free-form strings — they should display literally rather than becoming part of the surrounding markup.One-line change in
Cron_Logs::display(). The adjacent$task_nameand the$namelabel are both left alone on purpose:$task_nameisexplode( ':', $job->hook )[0], andget_plugin_cron_jobs()only ever queries the six hardcoded hook prefixes inManager::$wildcard_cron_tasks, so it can only be one of those literals;$namenever leaves the array literal it's declared in.Tests
Cron_Logs_Metabox_Testrenders the real metabox through an output buffer and asserts on the markup, rather than testing a helper in isolation. Three of its five cases fail without the change.Reaching
display()needs a job source, sotests/fixtures/cavalcade/class-job.phpstands in for Cavalcade's job model, which isn't present in the test install.Managergates every Cavalcade call onclass_exists(), and nothing else in this suite exercises those paths, so defining the stub doesn't affect other tests.Full suite passes (338 tests, 950 assertions), and
phpcsis clean on all three files.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests