Refactor: Extract CBT_Theme_Save service from rest_save_theme#833
Conversation
`! empty()` treats the string "false" as truthy because it is non-empty, so a caller sending `saveFonts=false` over a query string or form-encoded body would have its disabled flag silently flipped to enabled. The strict `true ===` check it replaced did not have this problem in the falsy direction. `wp_validate_boolean()` is WordPress's idiomatic helper for the common forms of bool-ish input: it accepts `true`, `"true"` (case- insensitive), `1`, `"1"` as truthy and `false`, `"false"`, `0`, `"0"`, `null`, `""` as falsy. The null-coalesce `?? false` keeps the helper from receiving a PHP-8.x undefined-index notice when a key is missing. Three new cases added to the falsy data provider: `"false"`, `"FALSE"` (case-insensitivity), and `"0"`. Found in Codex review of #833.
Three documentation tweaks for #833 reviewer onboarding, no behavior change: - File-level docblock on `theme-save.php` describes the service's role and explicitly notes that callers — not the service itself — are responsible for capability and input sanitization checks. - `run()` docblock expands on the same point with concrete examples (`current_user_can( 'edit_theme_options' )`, downstream services that consume non-flag option values), so a future CLI implementer is not left guessing what the REST framework was doing on their behalf. - `test-theme-save.php` class-level docblock notes that cache invalidation is intentionally not asserted, and why, so a future contributor does not file it as a coverage gap.
Move the orchestration logic out of `rest_save_theme()` into a new `CBT_Theme_Save::run()` static service so a future `wp cbt save` CLI command can share the same code path. The REST endpoint becomes a thin wrapper; URL, permissions, and response shape are unchanged. Two latent bugs in the orchestrator are fixed by the same one-line helper that normalizes the save flags: - `processOnlySavedTemplates` was dereferenced without an `isset()` guard, raising an `Undefined index` notice when `saveTemplates` was passed without the companion flag. - Flags were compared with strict `true ===`, rejecting `1` / `"true"` / `"1"` from non-JS callers (CLI, third-party REST clients). A single `! empty()` pass through `normalize_flags()` collapses both into one place. New PHPUnit coverage in `tests/test-theme-save.php`: - empty-options no-op + side-effect check - `processOnlySavedTemplates` no-notice regression (matcher narrowed to the key name so unrelated downstream notices do not fail it) - truthy/falsy flag normalization data providers - missing-key skip - `WP_Error` propagation from the patterns step Refs #828 Closes #829
`! empty()` treats the string "false" as truthy because it is non-empty, so a caller sending `saveFonts=false` over a query string or form-encoded body would have its disabled flag silently flipped to enabled. The strict `true ===` check it replaced did not have this problem in the falsy direction. `wp_validate_boolean()` is WordPress's idiomatic helper for the common forms of bool-ish input: it accepts `true`, `"true"` (case- insensitive), `1`, `"1"` as truthy and `false`, `"false"`, `0`, `"0"`, `null`, `""` as falsy. The null-coalesce `?? false` keeps the helper from receiving a PHP-8.x undefined-index notice when a key is missing. Three new cases added to the falsy data provider: `"false"`, `"FALSE"` (case-insensitivity), and `"0"`. Found in Codex review of #833.
Three documentation tweaks for #833 reviewer onboarding, no behavior change: - File-level docblock on `theme-save.php` describes the service's role and explicitly notes that callers — not the service itself — are responsible for capability and input sanitization checks. - `run()` docblock expands on the same point with concrete examples (`current_user_can( 'edit_theme_options' )`, downstream services that consume non-flag option values), so a future CLI implementer is not left guessing what the REST framework was doing on their behalf. - `test-theme-save.php` class-level docblock notes that cache invalidation is intentionally not asserted, and why, so a future contributor does not file it as a coverage gap.
dd3afaa to
562116b
Compare
There was a problem hiding this comment.
Pull request overview
This PR extracts the “save theme” orchestration out of the REST handler into a reusable CBT_Theme_Save::run() service, enabling the upcoming WP-CLI wp cbt save command to share the same save implementation while keeping the REST endpoint behavior and response shape unchanged.
Changes:
- Introduces
CBT_Theme_Saveservice to orchestrate fonts/templates/styles/patterns persistence with centralized flag normalization. - Refactors
rest_save_theme()into a thin wrapper around the new service. - Adds PHPUnit coverage for flag normalization regressions and key save behaviors (including WP_Error propagation).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
includes/create-theme/theme-save.php |
New service that contains the extracted save orchestration + flag normalization. |
includes/class-create-block-theme-api.php |
REST /save handler now delegates to CBT_Theme_Save::run() and preserves response shape. |
tests/test-theme-save.php |
New PHPUnit tests covering service behavior and regressions (undefined index + truthy flag handling). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Callers must enforce their own capability checks (e.g. | ||
| * `current_user_can( 'edit_theme_options' )`) and sanitize any non-flag | ||
| * values they place in `$options` that flow into downstream services | ||
| * such as `CBT_Theme_Patterns::add_patterns_to_theme()` and | ||
| * `CBT_Theme_Templates::add_templates_to_local()`. The REST endpoint | ||
| * relies on the route's `permission_callback` and the framework's | ||
| * sanitize layer; CLI and other callers must replicate that. |
The prior docblock referenced 'the framework's sanitize layer', which misleads: the /save route does not declare args with sanitize_callback, so no framework-level sanitization is applied. Reword to state that callers must sanitize non-flag values equivalently. Addresses reviewer feedback on #833.
|
@scruffian would appreciate a review here when you have a moment — this is the first refactor in the WP-CLI track (#828) and unblocks #830. No wire-level behavior change; the REST endpoint keeps the same URL, method, permission callback, and response shape. Just pushed dbc140a addressing the Copilot review comment about the docblock — the previous wording implied a REST framework sanitize layer that doesn't actually exist for this route ( |
…gate Trunk PR #850 replaced the inline current_user_can( 'edit_theme_options' ) closure on the /save route with CBT_Theme_API::can_modify_theme(), which requires super-admin on multisite in addition to the edit_theme_options capability. Update the docblock example so future CLI/non-REST callers know the current gate, not the pre-#850 one. Addresses reviewer feedback on #833.
Summary
Lift the orchestration logic out of
rest_save_theme()into a newCBT_Theme_Save::run()static service, so the upcomingwp cbt saveCLI command (#830) can share the same code path. The REST endpoint
becomes a thin wrapper.
This is the first PR in the WP-CLI track tracked in #828.
Changes
includes/create-theme/theme-save.php—CBT_Theme_Save::run( array $options ): true|WP_Error. Verbatimport of the four save branches (fonts, templates, styles, patterns)
includes/class-create-block-theme-api.php—rest_save_theme()shrunk from 50 lines to 14: callrun(),return
WP_Errordirectly, otherwise wrap success in the existingWP_REST_Responseshape.tests/test-theme-save.php— 13 PHPUnit cases.Bug fixes that fall out of the refactor
The orchestration logic had two latent issues that get resolved by
the new
normalize_flags()helper, which routes every flag through asingle
! empty()check:processOnlySavedTemplateswas previously dereferenced without anisset()guard, raising anUndefined indexnotice whensaveTemplateswas passed without it. The current Site Editor UIalways sends both flags, so the notice was only reachable from
non-JS callers — but the upcoming CLI is exactly that.
true ===, rejecting1,"true","1". This is fine for a typed JS client and brittlefor CLI and third-party REST callers.
Scope guarantees
No wire-level behavior change at the REST boundary:
{ status: 'SUCCESS', message: 'Theme Saved.' }).WP_Errorfrom the patterns step is still returned as-is — theREST framework converts it to a JSON error response with the same
status code as before.
The looser truthy semantics from the bug fix are isolated to the
/saveendpoint. Sibling spots in the same file(
rest_create_variation,rest_reset_theme) keep their existingstrict-
===checks; those endpoints already hadisset()guardsand never produced a notice. If we want consistency across the file,
that's worth a separate small PR.
Test plan
PHPUnit (added in this PR):
test_run_with_empty_options_returns_truetest_run_without_options_does_not_invoke_any_save_steptest_save_templates_without_process_only_flag_does_not_warn—regression test for the
processOnlySavedTemplatesnotice.Matcher narrowed to the key name so unrelated downstream notices
do not fail it.
test_truthy_save_fonts_values_trigger_persist(data provider:true,1,"1","true")test_falsy_save_fonts_values_skip_persist(data provider:false,0,"",null)test_missing_save_fonts_key_skips_persisttest_run_propagates_wp_error_from_patterns_stepManual smoke (Site Editor → Save Changes panel) covering the four
branches and the
WP_Errorpath:modified templates written.
templates written, no
Undefined indexnotice in the PHPerror log.
theme.jsonupdated, user-sideglobal styles cleared.
pattern_already_existssnackbar error fromWP_Error.Cache invalidation (
wp_get_theme()->cache_delete()) is not assertedin PHPUnit; observing it would require mocking statics, which isn't
worth a new test framework just for one assertion.
Out of scope
CBT_Theme_Fonts,CBT_Theme_Templates,CBT_Theme_JSON,CBT_Theme_Patterns,CBT_Theme_Styles).pattern is there but the WP-CLI track only needs save and export.
WP_Error— separateconcern, intentionally not broadened here.
Pre-existing bug noticed during testing (separate)
The strict error handler in the regression test surfaced an undefined
property access at
theme-templates.php:233($template->mediaread without a property check). Not introduced by this PR; worth a
small follow-up issue.
Refs #828
Closes #829