From 538e4dd3065b2576fedd97fdff20d150c068982a Mon Sep 17 00:00:00 2001 From: TallblokeUK Date: Sat, 29 Aug 2026 11:09:31 +0100 Subject: [PATCH] fix: catch duplicate declarations within a batch of snippets Two snippets declaring the same function could both be activated in one action, and the site fataled on the very next request. Each snippet was validated on its own, against whatever PHP had declared at that moment. That set does not include the other snippets in the same batch, so neither saw the other and both passed. The redeclaration then happened at runtime, inside eval(), where it is a fatal error rather than a catchable one: the front end and the admin both returned 500, leaving no way to reach the snippets screen and switch one off. Carry the identifiers claimed by accepted snippets through the batch, so the second snippet to claim a name is held back rather than activated alongside the first. Guarded declarations are unaffected, since a function_exists() check is already recorded as an exception and cannot fatal. Anonymous functions claim no name. Scripts and stylesheets are not considered at all, as they share no namespace with PHP. Relates to #230. --- 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 ) ); + } +}