fix: catch duplicate declarations within a batch of snippets

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<string, string[]>
+	 */
+	private array $claimed_identifiers = [];
+
 	/**
 	 * Class constructor.
 	 *
-	 * @param string $code Snippet code for parsing.
+	 * @param string                  $code                Snippet code for parsing.
+	 * @param array<string, string[]> $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( "<?php\n" . $this->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<string, string[]>
+	 */
+	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 );
 	}

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

tests/unit/Snippets/Batch_Activation_Test.php

@@ -0,0 +1,144 @@
+<?php
+
+namespace Code_Snippets\Tests;
+
+use Code_Snippets\Model\Snippet;
+use Code_Snippets\UnitTestCase;
+use function Code_Snippets\activate_snippets;
+use function Code_Snippets\get_snippet;
+use function Code_Snippets\save_snippet;
+
+/**
+ * Tests for validating a batch of snippets together.
+ *
+ * @group snippets
+ */
+class Batch_Activation_Test extends UnitTestCase {
+
+	/**
+	 * Store an inactive snippet and return it.
+	 *
+	 * @param string $scope Snippet scope.
+	 * @param string $code  Snippet code.
+	 *
+	 * @return Snippet
+	 */
+	private function make_snippet( string $scope, string $code ): Snippet {
+		$snippet = new Snippet();
+		$snippet->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 ) );
+	}
+}