From 3c5a76656ecbc2049239c2d6cedb8e5db8b06dff Mon Sep 17 00:00:00 2001 From: TallblokeUK Date: Sat, 29 Aug 2026 10:51:45 +0100 Subject: [PATCH 1/3] fix: only validate PHP when activating snippets in bulk Bulk activation ran every selected snippet through the PHP code validator, whatever its type: foreach ( $snippets as $snippet ) { $validator = new Validator( $snippet->code ); The other two callers guard this on the snippet being PHP; this one was missed. The validator does not check syntax. It looks for redeclarations of functions and classes that already exist in PHP, which says nothing meaningful about CSS or JavaScript. Read against a script, any function sharing a name with a PHP built-in looks like a redeclaration, so these were all refused: next, reset, current, key, count, sort, end, compact, extract, header Those are ordinary names in JavaScript. A carousel with next() and reset() could not be activated in bulk, while the same snippet activated perfectly well from its own toggle, because that path is guarded. It also failed silently. Invalid snippets are dropped from the batch rather than reported, so the snippet simply stayed inactive with nothing said, and one such snippet returned null for the whole call. Validate only PHP, matching the other two callers. Reported on the support forum as JavaScript being rejected by strict validation rules. The reporter's explanation was not right, but the experience was: their scripts would not activate. --- src/php/snippet-ops.php | 10 +- .../Bulk_Activate_Validation_Test.php | 140 ++++++++++++++++++ 2 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 tests/unit/Snippets/Bulk_Activate_Validation_Test.php diff --git a/src/php/snippet-ops.php b/src/php/snippet-ops.php index b3384e989..9e3c725c8 100644 --- a/src/php/snippet-ops.php +++ b/src/php/snippet-ops.php @@ -448,8 +448,14 @@ function activate_snippets( array $ids, ?bool $network = null ): ?array { $valid_snippets = []; foreach ( $snippets as $snippet ) { - $validator = new Validator( $snippet->code ); - $code_error = $validator->validate(); + // Only PHP is validated. The validator looks for redeclarations of + // existing PHP functions and classes, which says nothing useful about + // CSS or JavaScript: a script defining `next()` or `reset()` was read + // as redeclaring the PHP built-ins of those names and silently refused + // activation, while the same snippet activated fine on its own. + $code_error = 'php' === $snippet->type + ? ( new Validator( $snippet->code ) )->validate() + : null; if ( ! $code_error ) { $valid_ids[] = $snippet->id; diff --git a/tests/unit/Snippets/Bulk_Activate_Validation_Test.php b/tests/unit/Snippets/Bulk_Activate_Validation_Test.php new file mode 100644 index 000000000..e524e4b67 --- /dev/null +++ b/tests/unit/Snippets/Bulk_Activate_Validation_Test.php @@ -0,0 +1,140 @@ +name = 'Validation test'; + $snippet->scope = $scope; + $snippet->code = $code; + $snippet->active = false; + + return save_snippet( $snippet ); + } + + /** + * Whether a snippet is active, read back from storage. + * + * @param int $id Snippet identifier. + * + * @return bool + */ + private function is_active( int $id ): bool { + return (bool) get_snippet( $id )->active; + } + + /** + * JavaScript naming a PHP built-in can be bulk activated. + * + * `next` and `reset` are ordinary names in a script, and both are PHP + * functions, so the validator reported a redeclaration and the snippet was + * quietly left inactive. + * + * @return void + */ + public function test_javascript_naming_php_builtins_can_be_bulk_activated(): void { + $snippet = $this->make_snippet( + 'site-footer-js', + "function next() {\n\tindex += 1;\n}\n\nfunction reset() {\n\tindex = 0;\n}" + ); + + activate_snippets( [ $snippet->id ] ); + + $this->assertTrue( $this->is_active( $snippet->id ) ); + } + + /** + * The same snippet has always activated on its own, which is the + * inconsistency people run into. + * + * @return void + */ + public function test_single_activation_of_the_same_snippet_already_worked(): void { + $snippet = $this->make_snippet( 'site-footer-js', 'function reset() {}' ); + + activate_snippet( $snippet->id ); + + $this->assertTrue( $this->is_active( $snippet->id ) ); + } + + /** + * Stylesheets are not run through the PHP validator either. + * + * @return void + */ + public function test_stylesheets_can_be_bulk_activated(): void { + $snippet = $this->make_snippet( 'site-css', '.count { color: red; }' ); + + activate_snippets( [ $snippet->id ] ); + + $this->assertTrue( $this->is_active( $snippet->id ) ); + } + + /** + * PHP is still checked: a genuine redeclaration is still refused. + * + * @return void + */ + public function test_php_redeclaring_an_existing_function_is_still_refused(): void { + $snippet = $this->make_snippet( 'global', 'function get_option() { return 1; }' ); + + $result = activate_snippets( [ $snippet->id ] ); + + $this->assertNull( $result ); + $this->assertFalse( $this->is_active( $snippet->id ) ); + } + + /** + * PHP that is fine still activates. + * + * @return void + */ + public function test_valid_php_is_still_bulk_activated(): void { + $snippet = $this->make_snippet( 'global', "add_filter( 'the_content', 'cs_test_cb' );" ); + + activate_snippets( [ $snippet->id ] ); + + $this->assertTrue( $this->is_active( $snippet->id ) ); + } + + /** + * A bad PHP snippet does not prevent the others in the batch activating. + * + * @return void + */ + public function test_one_invalid_php_snippet_does_not_block_the_batch(): void { + $good = $this->make_snippet( 'site-footer-js', 'function count() {}' ); + $bad = $this->make_snippet( 'global', 'function get_option() { return 1; }' ); + + activate_snippets( [ $good->id, $bad->id ] ); + + $this->assertTrue( $this->is_active( $good->id ) ); + $this->assertFalse( $this->is_active( $bad->id ) ); + } +} From 27ce583223fde79732eb31fdc763820b2fc58c27 Mon Sep 17 00:00:00 2001 From: TallblokeUK Date: Sat, 29 Aug 2026 10:58:57 +0100 Subject: [PATCH 2/3] test: derive the colliding name from what is actually declared check_duplicate_identifier() builds its list from get_defined_functions(), so it covers PHP internals plus every function declared by WordPress, the active plugins and the theme. On a modest install that is over five and a half thousand names, four thousand of them from plugins. The set of JavaScript names that used to be refused was therefore specific to each site and grew as plugins were added, which is why the behaviour looked arbitrary and was hard to reproduce. Deriving the name from the live list rather than hard-coding one keeps the test honest whatever happens to be loaded. --- .../Bulk_Activate_Validation_Test.php | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/unit/Snippets/Bulk_Activate_Validation_Test.php b/tests/unit/Snippets/Bulk_Activate_Validation_Test.php index e524e4b67..135286bde 100644 --- a/tests/unit/Snippets/Bulk_Activate_Validation_Test.php +++ b/tests/unit/Snippets/Bulk_Activate_Validation_Test.php @@ -137,4 +137,39 @@ public function test_one_invalid_php_snippet_does_not_block_the_batch(): void { $this->assertTrue( $this->is_active( $good->id ) ); $this->assertFalse( $this->is_active( $bad->id ) ); } + + /** + * The rejected names come from whatever is declared, not a fixed list. + * + * `check_duplicate_identifier()` builds its list from + * `get_defined_functions()`, covering PHP internals and every function + * declared by WordPress, the active plugins and the theme. So the set of + * JavaScript names that used to be refused was specific to each site and + * grew as plugins were added, which is why the behaviour looked arbitrary + * and was hard to reproduce. + * + * Deriving the name here rather than hard-coding one keeps this honest + * whatever is loaded in the test environment. + * + * @return void + */ + public function test_a_name_declared_on_this_install_no_longer_blocks_javascript(): void { + $defined = get_defined_functions(); + $candidates = array_intersect( + [ 'next', 'reset', 'count', 'sort', 'log', 'min', 'max', 'trim' ], + array_map( 'strtolower', array_merge( $defined['internal'], $defined['user'] ) ) + ); + + $this->assertNotEmpty( $candidates, 'Expected at least one common name to be declared.' ); + + $name = (string) reset( $candidates ); + $snippet = $this->make_snippet( 'site-footer-js', "function $name() { return 1; }" ); + + activate_snippets( [ $snippet->id ] ); + + $this->assertTrue( + $this->is_active( $snippet->id ), + "A script declaring $name should still activate." + ); + } } From 391f61915f541ec3f20456621327c27ba624f266 Mon Sep 17 00:00:00 2001 From: TallblokeUK Date: Mon, 31 Aug 2026 18:45:59 +0100 Subject: [PATCH 3/3] fix: catch duplicate declarations within a batch of snippets (#492) --- src/php/Utils/Validator.php | 42 ++++- src/php/snippet-ops.php | 23 ++- tests/unit/Snippets/Batch_Activation_Test.php | 144 ++++++++++++++++++ 3 files changed, 198 insertions(+), 11 deletions(-) create mode 100644 tests/unit/Snippets/Batch_Activation_Test.php diff --git a/src/php/Utils/Validator.php b/src/php/Utils/Validator.php index d034f1c97..324605fb3 100644 --- a/src/php/Utils/Validator.php +++ b/src/php/Utils/Validator.php @@ -51,18 +51,46 @@ class Validator { */ private array $exceptions = []; + /** + * Identifiers already claimed by other snippets being validated alongside + * this one. + * + * A snippet is validated against everything PHP has declared so far, which + * does not include a snippet that is about to be activated in the same + * batch. Two snippets declaring the same function therefore both passed and + * both activated, and the site fataled on the next request. + * + * @var array + */ + private array $claimed_identifiers = []; + /** * Class constructor. * - * @param string $code Snippet code for parsing. + * @param string $code Snippet code for parsing. + * @param array $claimed_identifiers Identifiers already claimed by + * snippets validated alongside this one. */ - public function __construct( string $code ) { + public function __construct( string $code, array $claimed_identifiers = [] ) { + $this->claimed_identifiers = $claimed_identifiers; $this->code = $code; $this->tokens = token_get_all( "code ); $this->length = count( $this->tokens ); $this->current = 0; } + /** + * Retrieve the identifiers claimed so far, including this snippet's own. + * + * Pass the result to the next Validator in a batch so that two snippets + * cannot both claim the same name. + * + * @return array + */ + public function get_claimed_identifiers(): array { + return $this->claimed_identifiers; + } + /** * Determine whether the parser has reached the end of the list of tokens. * @@ -127,13 +155,19 @@ private function check_duplicate_identifier( string $type, string $identifier ): } } - $duplicate_identifier = in_array( $identifier, $this->defined_identifiers[ $type ], true ); - $duplicate_namespaced = in_array( $namespaced_identifier, $this->defined_identifiers[ $type ], true ); + $known = array_merge( + $this->defined_identifiers[ $type ], + $this->claimed_identifiers[ $type ] ?? [] + ); + + $duplicate_identifier = in_array( $identifier, $known, true ); + $duplicate_namespaced = in_array( $namespaced_identifier, $known, true ); $exceptions = $this->exceptions[ $type ] ?? []; $exception_identifier = in_array( $identifier, $exceptions, true ); $exception_namespaced = in_array( $namespaced_identifier, $exceptions, true ); array_unshift( $this->defined_identifiers[ $type ], $identifier ); + $this->claimed_identifiers[ $type ][] = $identifier; return ( $duplicate_identifier && ! $exception_identifier ) || ( $duplicate_namespaced && ! $exception_namespaced ); } diff --git a/src/php/snippet-ops.php b/src/php/snippet-ops.php index 9e3c725c8..deae3755a 100644 --- a/src/php/snippet-ops.php +++ b/src/php/snippet-ops.php @@ -447,17 +447,26 @@ function activate_snippets( array $ids, ?bool $network = null ): ?array { $valid_ids = []; $valid_snippets = []; + // Names claimed by snippets already accepted into this batch. A snippet is + // otherwise validated only against what PHP has declared so far, which does + // not include the other snippets about to be activated alongside it. + $claimed_identifiers = []; + foreach ( $snippets as $snippet ) { // Only PHP is validated. The validator looks for redeclarations of - // existing PHP functions and classes, which says nothing useful about - // CSS or JavaScript: a script defining `next()` or `reset()` was read - // as redeclaring the PHP built-ins of those names and silently refused - // activation, while the same snippet activated fine on its own. - $code_error = 'php' === $snippet->type - ? ( new Validator( $snippet->code ) )->validate() - : null; + // existing PHP functions and classes, which says nothing meaningful + // about CSS or JavaScript. + if ( 'php' !== $snippet->type ) { + $valid_ids[] = $snippet->id; + $valid_snippets[] = $snippet; + continue; + } + + $validator = new Validator( $snippet->code, $claimed_identifiers ); + $code_error = $validator->validate(); if ( ! $code_error ) { + $claimed_identifiers = $validator->get_claimed_identifiers(); $valid_ids[] = $snippet->id; $valid_snippets[] = $snippet; } diff --git a/tests/unit/Snippets/Batch_Activation_Test.php b/tests/unit/Snippets/Batch_Activation_Test.php new file mode 100644 index 000000000..fa1f4dc8d --- /dev/null +++ b/tests/unit/Snippets/Batch_Activation_Test.php @@ -0,0 +1,144 @@ +name = 'Batch test'; + $snippet->scope = $scope; + $snippet->code = $code; + $snippet->active = false; + + return save_snippet( $snippet ); + } + + /** + * Whether a snippet is active, read back from storage. + * + * @param int $id Snippet identifier. + * + * @return bool + */ + private function is_active( int $id ): bool { + return (bool) get_snippet( $id )->active; + } + + /** + * Two snippets declaring the same function are not both activated. + * + * Each was previously validated only against what PHP had declared at the + * time, which did not include the other snippet in the same batch. Both + * passed, both activated, and the next request fataled with + * "Cannot redeclare function". + * + * @return void + */ + public function test_two_snippets_declaring_the_same_function_do_not_both_activate(): void { + $first = $this->make_snippet( 'global', 'function cs_batch_helper() { return 1; }' ); + $second = $this->make_snippet( 'global', 'function cs_batch_helper() { return 2; }' ); + + activate_snippets( [ $first->id, $second->id ] ); + + $this->assertTrue( $this->is_active( $first->id ), 'The first snippet should activate.' ); + $this->assertFalse( $this->is_active( $second->id ), 'The second should be held back.' ); + } + + /** + * The same applies to classes. + * + * @return void + */ + public function test_two_snippets_declaring_the_same_class_do_not_both_activate(): void { + $first = $this->make_snippet( 'global', 'class CS_Batch_Widget {}' ); + $second = $this->make_snippet( 'global', 'class CS_Batch_Widget {}' ); + + activate_snippets( [ $first->id, $second->id ] ); + + $this->assertTrue( $this->is_active( $first->id ) ); + $this->assertFalse( $this->is_active( $second->id ) ); + } + + /** + * Snippets declaring different names both activate. + * + * @return void + */ + public function test_snippets_with_different_names_both_activate(): void { + $first = $this->make_snippet( 'global', 'function cs_batch_one() { return 1; }' ); + $second = $this->make_snippet( 'global', 'function cs_batch_two() { return 2; }' ); + + activate_snippets( [ $first->id, $second->id ] ); + + $this->assertTrue( $this->is_active( $first->id ) ); + $this->assertTrue( $this->is_active( $second->id ) ); + } + + /** + * A guarded redeclaration is still allowed, as it cannot fatal. + * + * @return void + */ + public function test_guarded_declarations_are_allowed(): void { + $first = $this->make_snippet( 'global', 'function cs_batch_guarded() { return 1; }' ); + $second = $this->make_snippet( + 'global', + "if ( ! function_exists( 'cs_batch_guarded' ) ) {\n\tfunction cs_batch_guarded() { return 2; }\n}" + ); + + activate_snippets( [ $first->id, $second->id ] ); + + $this->assertTrue( $this->is_active( $first->id ) ); + $this->assertTrue( $this->is_active( $second->id ) ); + } + + /** + * Scripts are not held back by a name another snippet declares. + * + * @return void + */ + public function test_scripts_are_unaffected_by_php_names(): void { + $php = $this->make_snippet( 'global', 'function cs_batch_shared() { return 1; }' ); + $js = $this->make_snippet( 'site-footer-js', 'function cs_batch_shared() { return 2; }' ); + + activate_snippets( [ $php->id, $js->id ] ); + + $this->assertTrue( $this->is_active( $php->id ) ); + $this->assertTrue( $this->is_active( $js->id ), 'JavaScript shares no namespace with PHP.' ); + } + + /** + * Anonymous functions do not claim a name. + * + * @return void + */ + public function test_anonymous_functions_do_not_collide(): void { + $first = $this->make_snippet( 'global', "add_filter( 'the_content', function ( \$c ) { return \$c; } );" ); + $second = $this->make_snippet( 'global', "add_filter( 'the_title', function ( \$t ) { return \$t; } );" ); + + activate_snippets( [ $first->id, $second->id ] ); + + $this->assertTrue( $this->is_active( $first->id ) ); + $this->assertTrue( $this->is_active( $second->id ) ); + } +}