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 ) ); + } +}