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 b3384e989..deae3755a 100644 --- a/src/php/snippet-ops.php +++ b/src/php/snippet-ops.php @@ -447,11 +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 ) { - $validator = new Validator( $snippet->code ); + // Only PHP is validated. The validator looks for redeclarations of + // 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 ) ); + } +} 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..135286bde --- /dev/null +++ b/tests/unit/Snippets/Bulk_Activate_Validation_Test.php @@ -0,0 +1,175 @@ +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 ) ); + } + + /** + * 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." + ); + } +}