From 8f093cf14a477dcc65b3e8470d62bf6a93276b14 Mon Sep 17 00:00:00 2001 From: Birgit Pauli-Haack Date: Tue, 5 May 2026 16:57:50 +0200 Subject: [PATCH 1/3] Refactor: extract CBT_Theme_Save service from rest_save_theme 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 --- includes/class-create-block-theme-api.php | 41 +--- includes/create-theme/theme-save.php | 82 ++++++++ tests/test-theme-save.php | 238 ++++++++++++++++++++++ 3 files changed, 324 insertions(+), 37 deletions(-) create mode 100644 includes/create-theme/theme-save.php create mode 100644 tests/test-theme-save.php diff --git a/includes/class-create-block-theme-api.php b/includes/class-create-block-theme-api.php index 12c42522..0127653e 100644 --- a/includes/class-create-block-theme-api.php +++ b/includes/class-create-block-theme-api.php @@ -13,6 +13,7 @@ require_once __DIR__ . '/create-theme/theme-readme.php'; require_once __DIR__ . '/create-theme/theme-fonts.php'; require_once __DIR__ . '/create-theme/theme-create.php'; +require_once __DIR__ . '/create-theme/theme-save.php'; /** * The api functionality of the plugin leveraged by the site editor UI. @@ -346,46 +347,12 @@ function rest_update_theme( $request ) { * Save the user changes to the theme and clear user changes. */ function rest_save_theme( $request ) { + $result = CBT_Theme_Save::run( $request->get_params() ); - $options = $request->get_params(); - - if ( isset( $options['saveFonts'] ) && true === $options['saveFonts'] ) { - CBT_Theme_Fonts::persist_font_settings(); + if ( is_wp_error( $result ) ) { + return $result; } - if ( isset( $options['saveTemplates'] ) && true === $options['saveTemplates'] ) { - if ( true === $options['processOnlySavedTemplates'] ) { - CBT_Theme_Templates::add_templates_to_local( 'user', null, null, $options ); - } else { - if ( is_child_theme() ) { - CBT_Theme_Templates::add_templates_to_local( 'current', null, null, $options ); - } else { - CBT_Theme_Templates::add_templates_to_local( 'all', null, null, $options ); - } - } - CBT_Theme_Templates::clear_user_templates_customizations(); - CBT_Theme_Templates::clear_user_template_parts_customizations(); - } - - if ( isset( $options['saveStyle'] ) && true === $options['saveStyle'] ) { - if ( is_child_theme() ) { - CBT_Theme_JSON::add_theme_json_to_local( 'current', null, null, $options ); - } else { - CBT_Theme_JSON::add_theme_json_to_local( 'all', null, null, $options ); - } - CBT_Theme_Styles::clear_user_styles_customizations(); - } - - if ( isset( $options['savePatterns'] ) && true === $options['savePatterns'] ) { - $response = CBT_Theme_Patterns::add_patterns_to_theme( $options ); - - if ( is_wp_error( $response ) ) { - return $response; - } - } - - wp_get_theme()->cache_delete(); - return new WP_REST_Response( array( 'status' => 'SUCCESS', diff --git a/includes/create-theme/theme-save.php b/includes/create-theme/theme-save.php new file mode 100644 index 00000000..e99fcb69 --- /dev/null +++ b/includes/create-theme/theme-save.php @@ -0,0 +1,82 @@ +cache_delete(); + + return true; + } + + /** + * Normalize the save flags through a single ! empty() check. + * + * Collapses two prior issues: undefined-index notices when a flag was + * read without isset() (notably processOnlySavedTemplates), and strict + * `true ===` checks that rejected `1` / `"true"` / `"1"` from non-JS + * callers. + * + * @param array $options Raw options array. + * @return array Normalized boolean flags. + */ + private static function normalize_flags( array $options ) { + return array( + 'saveFonts' => ! empty( $options['saveFonts'] ), + 'saveTemplates' => ! empty( $options['saveTemplates'] ), + 'processOnlySavedTemplates' => ! empty( $options['processOnlySavedTemplates'] ), + 'saveStyle' => ! empty( $options['saveStyle'] ), + 'savePatterns' => ! empty( $options['savePatterns'] ), + ); + } +} diff --git a/tests/test-theme-save.php b/tests/test-theme-save.php new file mode 100644 index 00000000..98bac1ca --- /dev/null +++ b/tests/test-theme-save.php @@ -0,0 +1,238 @@ +user->create( + array( + 'role' => 'administrator', + ) + ); + } + + public function set_up() { + parent::set_up(); + wp_set_current_user( self::$admin_id ); + } + + public function test_run_with_empty_options_returns_true() { + $result = CBT_Theme_Save::run( array() ); + $this->assertTrue( $result ); + } + + public function test_run_without_options_does_not_invoke_any_save_step() { + $test_theme_slug = $this->create_blank_theme(); + + // Stage a user font that would be moved into the theme if saveFonts ran. + $this->stage_user_font(); + $user_settings_before = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); + + CBT_Theme_Save::run( array() ); + + $user_settings_after = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); + + // User settings untouched: persist_font_settings was not called. + $this->assertEquals( $user_settings_before, $user_settings_after ); + + $this->uninstall_theme( $test_theme_slug ); + } + + /** + * Regression: previously `true === $options['processOnlySavedTemplates']` + * was read without isset(), which raised an Undefined index notice when + * saveTemplates was passed without processOnlySavedTemplates. + * + * The matcher is intentionally narrowed to the key name so unrelated + * pre-existing notices/warnings in downstream code do not fail this test. + */ + public function test_save_templates_without_process_only_flag_does_not_warn() { + $test_theme_slug = $this->create_blank_theme(); + + set_error_handler( + static function ( $errno, $errstr ) { + if ( false !== strpos( $errstr, 'processOnlySavedTemplates' ) ) { + throw new \ErrorException( $errstr, 0, $errno ); + } + return false; + } + ); + + try { + $result = CBT_Theme_Save::run( array( 'saveTemplates' => true ) ); + } finally { + restore_error_handler(); + } + + $this->assertTrue( $result ); + + $this->uninstall_theme( $test_theme_slug ); + } + + /** + * @dataProvider truthy_flag_provider + */ + public function test_truthy_save_fonts_values_trigger_persist( $truthy_value ) { + $test_theme_slug = $this->create_blank_theme(); + $this->stage_user_font(); + + // Sanity: user font is staged before run(). + $user_settings_before = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); + $this->assertArrayHasKey( 'typography', $user_settings_before ); + + CBT_Theme_Save::run( array( 'saveFonts' => $truthy_value ) ); + + // persist_font_settings clears user-space typography after moving it into the theme. + $user_settings_after = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); + $this->assertArrayNotHasKey( + 'typography', + $user_settings_after, + 'saveFonts flag value did not trigger persist' + ); + + $this->uninstall_theme( $test_theme_slug ); + } + + public function truthy_flag_provider() { + return array( + 'bool true' => array( true ), + 'int 1' => array( 1 ), + 'string "1"' => array( '1' ), + 'string "true"' => array( 'true' ), + ); + } + + /** + * @dataProvider falsy_flag_provider + */ + public function test_falsy_save_fonts_values_skip_persist( $falsy_value ) { + $test_theme_slug = $this->create_blank_theme(); + $this->stage_user_font(); + + $options = array( 'saveFonts' => $falsy_value ); + CBT_Theme_Save::run( $options ); + + // User typography should remain in place because persist did not run. + $user_settings_after = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); + $this->assertArrayHasKey( + 'typography', + $user_settings_after, + 'falsy saveFonts value should not have triggered persist' + ); + + $this->uninstall_theme( $test_theme_slug ); + } + + public function falsy_flag_provider() { + return array( + 'bool false' => array( false ), + 'int 0' => array( 0 ), + 'empty string' => array( '' ), + 'null' => array( null ), + ); + } + + public function test_missing_save_fonts_key_skips_persist() { + $test_theme_slug = $this->create_blank_theme(); + $this->stage_user_font(); + + CBT_Theme_Save::run( array( 'saveStyle' => false ) ); + + $user_settings_after = CBT_Theme_JSON_Resolver::get_user_data()->get_settings(); + $this->assertArrayHasKey( 'typography', $user_settings_after ); + + $this->uninstall_theme( $test_theme_slug ); + } + + public function test_run_propagates_wp_error_from_patterns_step() { + $test_theme_slug = $this->create_blank_theme(); + + // Create a wp_block post; CBT_Theme_Patterns will try to write a file + // for it under the theme's patterns/ directory. + $pattern_name = 'cbt-test-conflict-pattern'; + self::factory()->post->create( + array( + 'post_type' => 'wp_block', + 'post_status' => 'publish', + 'post_title' => $pattern_name, + 'post_content' => '

conflict

', + ) + ); + + // Pre-create a file with the same pattern slug so the second write conflicts. + $patterns_dir = get_stylesheet_directory() . DIRECTORY_SEPARATOR . 'patterns'; + if ( ! is_dir( $patterns_dir ) ) { + wp_mkdir_p( $patterns_dir ); + } + file_put_contents( $patterns_dir . DIRECTORY_SEPARATOR . $pattern_name . '.php', ' true ) ); + + $this->assertWPError( $result ); + $this->assertEquals( 'pattern_already_exists', $result->get_error_code() ); + + $this->uninstall_theme( $test_theme_slug ); + } + + private function create_blank_theme() { + $test_theme_slug = 'cbttesttheme'; + + delete_theme( $test_theme_slug ); + + $request = new WP_REST_Request( 'POST', '/create-block-theme/v1/create-blank' ); + $request->set_param( 'name', $test_theme_slug ); + $request->set_param( 'description', '' ); + $request->set_param( 'uri', '' ); + $request->set_param( 'author', '' ); + $request->set_param( 'author_uri', '' ); + $request->set_param( 'tags_custom', '' ); + $request->set_param( 'recommended_plugins', '' ); + + rest_do_request( $request ); + + CBT_Theme_JSON_Resolver::clean_cached_data(); + + return $test_theme_slug; + } + + private function uninstall_theme( $theme_slug ) { + CBT_Theme_JSON_Resolver::write_user_settings( array() ); + delete_theme( $theme_slug ); + } + + private function stage_user_font() { + $font_dir = wp_get_font_dir(); + $font_test_source = __DIR__ . '/data/fonts/OpenSans-Regular.ttf'; + $font_test_url = $font_dir['url'] . '/open-sans-normal-400.ttf'; + $font_test_destination = $font_dir['path'] . '/open-sans-normal-400.ttf'; + + if ( ! file_exists( $font_dir['path'] ) ) { + mkdir( $font_dir['path'] ); + } + copy( $font_test_source, $font_test_destination ); + + $settings = array(); + $settings['typography']['fontFamilies']['custom'] = array( + array( + 'slug' => 'open-sans', + 'name' => 'Open Sans', + 'fontFamily' => 'Open Sans', + 'fontFace' => array( + array( + 'fontFamily' => 'Open Sans', + 'fontStyle' => 'normal', + 'fontWeight' => '400', + 'src' => $font_test_url, + ), + ), + ), + ); + + CBT_Theme_JSON_Resolver::write_user_settings( $settings ); + } +} From 1607a944d2fa2f040d2d83a5dcab429c47b1c20b Mon Sep 17 00:00:00 2001 From: Birgit Pauli-Haack Date: Wed, 6 May 2026 08:34:37 +0200 Subject: [PATCH 2/3] Use wp_validate_boolean() in normalize_flags() `! 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. --- includes/create-theme/theme-save.php | 18 +++++++++++------- tests/test-theme-save.php | 11 +++++++---- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/includes/create-theme/theme-save.php b/includes/create-theme/theme-save.php index e99fcb69..ab499b90 100644 --- a/includes/create-theme/theme-save.php +++ b/includes/create-theme/theme-save.php @@ -60,23 +60,27 @@ public static function run( array $options ) { } /** - * Normalize the save flags through a single ! empty() check. + * Normalize the save flags through wp_validate_boolean(). * * Collapses two prior issues: undefined-index notices when a flag was * read without isset() (notably processOnlySavedTemplates), and strict * `true ===` checks that rejected `1` / `"true"` / `"1"` from non-JS - * callers. + * callers (CLI, form-encoded REST clients). + * + * Uses wp_validate_boolean() rather than ! empty() so the string + * "false" (commonly sent by query-string and form-encoded callers) is + * treated as falsy, not truthy. * * @param array $options Raw options array. * @return array Normalized boolean flags. */ private static function normalize_flags( array $options ) { return array( - 'saveFonts' => ! empty( $options['saveFonts'] ), - 'saveTemplates' => ! empty( $options['saveTemplates'] ), - 'processOnlySavedTemplates' => ! empty( $options['processOnlySavedTemplates'] ), - 'saveStyle' => ! empty( $options['saveStyle'] ), - 'savePatterns' => ! empty( $options['savePatterns'] ), + 'saveFonts' => wp_validate_boolean( $options['saveFonts'] ?? false ), + 'saveTemplates' => wp_validate_boolean( $options['saveTemplates'] ?? false ), + 'processOnlySavedTemplates' => wp_validate_boolean( $options['processOnlySavedTemplates'] ?? false ), + 'saveStyle' => wp_validate_boolean( $options['saveStyle'] ?? false ), + 'savePatterns' => wp_validate_boolean( $options['savePatterns'] ?? false ), ); } } diff --git a/tests/test-theme-save.php b/tests/test-theme-save.php index 98bac1ca..f8cf3af1 100644 --- a/tests/test-theme-save.php +++ b/tests/test-theme-save.php @@ -130,10 +130,13 @@ public function test_falsy_save_fonts_values_skip_persist( $falsy_value ) { public function falsy_flag_provider() { return array( - 'bool false' => array( false ), - 'int 0' => array( 0 ), - 'empty string' => array( '' ), - 'null' => array( null ), + 'bool false' => array( false ), + 'int 0' => array( 0 ), + 'empty string' => array( '' ), + 'null' => array( null ), + 'string "false"' => array( 'false' ), + 'string "FALSE"' => array( 'FALSE' ), + 'string "0"' => array( '0' ), ); } From 754202325e20b5882f524ea5a16047cec6e1aef8 Mon Sep 17 00:00:00 2001 From: Birgit Pauli-Haack Date: Wed, 6 May 2026 10:12:00 +0200 Subject: [PATCH 3/3] Documentation: spell out caller responsibilities and test omission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- includes/create-theme/theme-save.php | 14 ++++++++++++++ tests/test-theme-save.php | 5 +++++ 2 files changed, 19 insertions(+) diff --git a/includes/create-theme/theme-save.php b/includes/create-theme/theme-save.php index ab499b90..3c5349fe 100644 --- a/includes/create-theme/theme-save.php +++ b/includes/create-theme/theme-save.php @@ -2,6 +2,12 @@ /** * Theme Save * + * Service that persists user changes from the Editor to the active theme on + * disk. Shared by the REST endpoint at `/create-block-theme/v1/save` and any + * future non-REST callers (e.g. WP-CLI). The service does not perform + * capability or input-sanitization checks on its own — callers are + * responsible for those before invoking `run()`. + * * @package Create_Block_Theme */ class CBT_Theme_Save { @@ -15,6 +21,14 @@ class CBT_Theme_Save { * `processOnlySavedTemplates` and `is_child_theme()`. After all steps run, * the theme cache is invalidated once. * + * 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. + * * @param array $options Options array with keys: * - saveFonts (bool) * - saveTemplates (bool) diff --git a/tests/test-theme-save.php b/tests/test-theme-save.php index f8cf3af1..20ebff3c 100644 --- a/tests/test-theme-save.php +++ b/tests/test-theme-save.php @@ -2,6 +2,11 @@ /** * Tests for CBT_Theme_Save service. * + * Note: cache invalidation (`wp_get_theme()->cache_delete()`) is intentionally + * not asserted here. Observing it would require either mocking the static + * `wp_get_theme()` helper or wiring up a counter via filters — both + * disproportionate to the value of locking in a single line of behavior. + * * @package Create_Block_Theme */ class Test_Create_Block_Theme_Save extends WP_UnitTestCase {