Plugin Directory: Delay trunk-only imports on tag-releasing plugins

wordpress.org/public_html/wp-content/plugins/plugin-directory/cli/class-svn-watcher.php

@@ -155,8 +155,8 @@ protected function get_plugin_changes_between( $rev, $head_rev = 'HEAD' ) {
 
 				}
 
-				// This will have false-positives for when a readme in a subdirectory is hit, but this is only for optimizations.
-				if ( in_array( strtolower( basename( $path ) ), array( 'readme.txt', 'readme.md' ) ) ) {
+				// Only count the readme at the root of /trunk or a tag — a readme.txt in a subdirectory is just bundled documentation.
+				if ( preg_match( '!/(trunk|tags/[^/]+)/readme\.(txt|md)$!i', $path ) ) {
 					$plugin['readme_touched'] = true;
 				}
 				if ( ! $plugin['code_touched'] && ( '/' === substr( $path, -1 ) || '.php' === substr( $path, -4 ) || '.js' === substr( $path, -3 ) ) ) {

wordpress.org/public_html/wp-content/plugins/plugin-directory/jobs/class-plugin-import.php

@@ -3,6 +3,8 @@
 
 use Exception;
 use WordPressdotorg\Plugin_Directory\CLI;
+use WordPressdotorg\Plugin_Directory\Plugin_Directory;
+use WordPressdotorg\Plugin_Directory\Readme\Parser as Readme_Parser;
 
 /**
  * Import plugin changes into WordPress.
@@ -11,24 +13,52 @@
  */
 class Plugin_Import {
 
+	/**
+	 * Queue an import job for a plugin, merging into any pending future event for the
+	 * same plugin so a single import covers all the SVN changes seen so far.
+	 *
+	 * @param string $plugin_slug The plugin slug.
+	 * @param array  $plugin_data Data about the SVN change (tags_touched, revisions, etc).
+	 */
 	public static function queue( $plugin_slug, $plugin_data ) {
+		$new_args = array_merge( array( 'plugin' => $plugin_slug ), $plugin_data );
+
 		/*
-		 * If the next scheduled run is more than 5 minutes away (e.g. queued by a bulk
-		 * batch re-index) and no import is currently running, pull it forward to the
-		 * usual import time so a fresh commit isn't delayed behind the batch. The new
-		 * commit-driven event is then queued 1hr later via the logic below.
+		 * If there's already a future-scheduled import for this plugin and nothing
+		 * is currently running, fold the new request into it. This handles two
+		 * cases under one rule:
+		 *
+		 *  - A bulk batch re-index queued an event far in the future; a fresh
+		 *    commit should pull that forward to the usual import time.
+		 *  - The 15-minute trunk grace window (see queue_run_time()): an author
+		 *    commits to /trunk first and then `svn cp trunk tags/X.Y` a moment
+		 *    later. The first commit's event is delayed; the second merges into
+		 *    it so a single import publishes from the tag, not from a trunk
+		 *    fallback that the tag commit then has to overwrite.
 		 */
 		$next_scheduled = Manager::get_scheduled_time( "import_plugin:{$plugin_slug}", 'next' );
-		if (
-			$next_scheduled &&
-			$next_scheduled > ( time() + 5 * MINUTE_IN_SECONDS ) &&
-			! Manager::is_event_running( "import_plugin:{$plugin_slug}" )
-		) {
-			Manager::reschedule_event( "import_plugin:{$plugin_slug}", time() + 5, $next_scheduled );
+		if ( $next_scheduled && ! Manager::is_event_running( "import_plugin:{$plugin_slug}" ) ) {
+			$existing      = Manager::get_scheduled_events( "import_plugin:{$plugin_slug}", $next_scheduled );
+			$existing_args = $existing[0]['args'][0] ?? array();
+			$merged_args   = self::merge_plugin_data( $existing_args, $new_args );
+
+			$updated = Manager::update_scheduled_event(
+				"import_plugin:{$plugin_slug}",
+				$next_scheduled,
+				array(
+					'nextrun' => min( $next_scheduled, self::queue_run_time( $plugin_slug, $merged_args ) ),
+					'args'    => array( $merged_args ),
+				)
+			);
+
+			if ( $updated ) {
+				return;
+			}
 		}
 
-		// To avoid a situation where two imports run concurrently, if one is already scheduled or in flight, run it 1hr later (We'll trigger it after the current one finishes).
-		$when_to_run    = time() + 5;
+		$when_to_run = self::queue_run_time( $plugin_slug, $new_args );
+
+		// To avoid a situation where two imports run concurrently, if one is already scheduled or in flight, run it 1hr later (we'll trigger it after the current one finishes).
 		$last_scheduled = Manager::get_scheduled_time( "import_plugin:{$plugin_slug}", 'last' );
 		if ( $last_scheduled ) {
 			$when_to_run = $last_scheduled + HOUR_IN_SECONDS;
@@ -39,12 +69,114 @@ public static function queue( $plugin_slug, $plugin_data ) {
 		wp_schedule_single_event(
 			$when_to_run,
 			"import_plugin:{$plugin_slug}",
-			array(
-				array_merge( array( 'plugin' => $plugin_slug ), $plugin_data ),
-			)
+			array( $new_args )
 		);
 	}
 
+	/**
+	 * Decide when an import job should run, based on the SVN changes it covers.
+	 *
+	 * Authors that release from a tag commonly commit the version bump to /trunk
+	 * first and then `svn cp trunk tags/X.Y` a moment later. Running the import
+	 * immediately on the trunk commit publishes a trunk-fallback release that
+	 * the follow-up tag commit then re-publishes from the tag. To collapse the
+	 * two into one import, trunk-only updates are deferred by 15 minutes — that
+	 * gives the follow-up tag commit time to merge into the same job (see
+	 * queue()). Tag-touching changes (additions or deletions) run immediately.
+	 *
+	 * The grace window is bypassed when the only change is a `Stable Tag` flip
+	 * in /trunk/readme.txt pointing at a tag that already exists in /tags/:
+	 * there's no follow-up tag to wait for in that case.
+	 *
+	 * @param string $plugin_slug The plugin slug.
+	 * @param array  $args        The args for the import job (post-merge where applicable).
+	 * @return int Unix timestamp for when the event should run.
+	 */
+	protected static function queue_run_time( $plugin_slug, $args ) {
+		if ( ! self::is_trunk_only_update( $args ) ) {
+			return time() + 5;
+		}
+
+		if ( ! empty( $args['readme_touched'] ) && self::trunk_stable_tag_flip_to_existing_tag( $plugin_slug ) ) {
+			return time() + 5;
+		}
+
+		return time() + 15 * MINUTE_IN_SECONDS;
+	}
+
+	/**
+	 * Whether an import only covers /trunk (no tags added or removed).
+	 *
+	 * @param array $args The args for the import job.
+	 * @return bool
+	 */
+	protected static function is_trunk_only_update( $args ) {
+		$tags_touched = (array) ( $args['tags_touched'] ?? array() );
+		$tags_deleted = (array) ( $args['tags_deleted'] ?? array() );
+
+		return $tags_touched && array( 'trunk' ) === array_values( array_unique( $tags_touched ) ) && ! $tags_deleted;
+	}
+
+	/**
+	 * Whether /trunk/readme.txt's Stable Tag points to a tag that's already in
+	 * /tags/ AND differs from the directory's current stable_tag.
+	 *
+	 * Indicates a release-by-readme-flip — the author isn't going to follow up
+	 * with `svn cp trunk tags/X.Y` because the tag already exists. Used to skip
+	 * the 15-minute trunk grace window in that case.
+	 *
+	 * @param string $plugin_slug The plugin slug.
+	 * @return bool
+	 */
+	protected static function trunk_stable_tag_flip_to_existing_tag( $plugin_slug ) {
+		$plugin = Plugin_Directory::get_plugin_post( $plugin_slug );
+		if ( ! $plugin ) {
+			return false;
+		}
+
+		$readme         = new Readme_Parser( "https://plugins.svn.wordpress.org/{$plugin_slug}/trunk/readme.txt" );
+		$new_stable_tag = $readme->stable_tag;
+
+		if ( ! $new_stable_tag || 'trunk' === $new_stable_tag ) {
+			return false;
+		}
+
+		if ( get_post_meta( $plugin->ID, 'stable_tag', true ) === $new_stable_tag ) {
+			return false;
+		}
+
+		$tagged_versions = (array) get_post_meta( $plugin->ID, 'tagged_versions', true );
+
+		return in_array( $new_stable_tag, $tagged_versions, true );
+	}
+
+	/**
+	 * Merge two plugin_data payloads into a single import-job payload.
+	 *
+	 * Used when folding an already-scheduled future event into a newer request so
+	 * neither set of changes is lost.
+	 *
+	 * @param array $existing The args from the currently-scheduled event.
+	 * @param array $incoming The args from the new request.
+	 * @return array Merged args ready to pass to the import job.
+	 */
+	protected static function merge_plugin_data( array $existing, array $incoming ) {
+		$merged = array_merge( $existing, $incoming );
+
+		foreach ( array( 'tags_touched', 'tags_deleted', 'revisions' ) as $key ) {
+			$existing_values = (array) ( $existing[ $key ] ?? array() );
+			$incoming_values = (array) ( $incoming[ $key ] ?? array() );
+
+			$merged[ $key ] = array_values( array_unique( array_merge( $existing_values, $incoming_values ) ) );
+		}
+
+		foreach ( array( 'readme_touched', 'code_touched', 'assets_touched' ) as $key ) {
+			$merged[ $key ] = ! empty( $existing[ $key ] ) || ! empty( $incoming[ $key ] );
+		}
+
+		return $merged;
+	}
+
 	/**
 	 * The cron trigger for the import job.
 	 */

wordpress.org/public_html/wp-content/plugins/plugin-directory/tests/Plugin_Import_Merge_Test.php

@@ -0,0 +1,146 @@
+<?php
+/**
+ * Tests for Plugin_Import::merge_plugin_data() and is_trunk_only_update() helpers.
+ *
+ * @package WordPressdotorg\Plugin_Directory\Tests
+ */
+
+use PHPUnit\Framework\TestCase;
+use WordPressdotorg\Plugin_Directory\Jobs\Plugin_Import;
+
+/**
+ * @group jobs
+ */
+class Plugin_Import_Merge_Test extends TestCase {
+
+	/**
+	 * Invoke the protected Plugin_Import::merge_plugin_data() helper.
+	 */
+	private function merge( array $existing, array $incoming ): array {
+		$method = new ReflectionMethod( Plugin_Import::class, 'merge_plugin_data' );
+		$method->setAccessible( true );
+
+		return $method->invoke( null, $existing, $incoming );
+	}
+
+	/**
+	 * Invoke the protected Plugin_Import::is_trunk_only_update() helper.
+	 */
+	private function is_trunk_only( array $args ): bool {
+		$method = new ReflectionMethod( Plugin_Import::class, 'is_trunk_only_update' );
+		$method->setAccessible( true );
+
+		return $method->invoke( null, $args );
+	}
+
+	public function test_merges_revisions_and_tags_without_duplicates() {
+		$existing = [
+			'plugin'         => 'hello',
+			'tags_touched'   => [ 'trunk', '1.0' ],
+			'tags_deleted'   => [],
+			'revisions'      => [ 100, 101 ],
+			'readme_touched' => true,
+			'code_touched'   => false,
+			'assets_touched' => false,
+		];
+		$new = [
+			'plugin'         => 'hello',
+			'tags_touched'   => [ 'trunk', '2.0' ],
+			'tags_deleted'   => [ '0.9' ],
+			'revisions'      => [ 101, 200 ],
+			'readme_touched' => false,
+			'code_touched'   => true,
+			'assets_touched' => false,
+		];
+
+		$merged = $this->merge( $existing, $new );
+
+		$this->assertEqualsCanonicalizing( [ 'trunk', '1.0', '2.0' ], $merged['tags_touched'] );
+		$this->assertEqualsCanonicalizing( [ '0.9' ], $merged['tags_deleted'] );
+		$this->assertEqualsCanonicalizing( [ 100, 101, 200 ], $merged['revisions'] );
+	}
+
+	public function test_boolean_flags_are_ored() {
+		$existing = [
+			'readme_touched' => true,
+			'code_touched'   => false,
+			'assets_touched' => false,
+		];
+		$new = [
+			'readme_touched' => false,
+			'code_touched'   => true,
+			'assets_touched' => false,
+		];
+
+		$merged = $this->merge( $existing, $new );
+
+		$this->assertTrue( $merged['readme_touched'] );
+		$this->assertTrue( $merged['code_touched'] );
+		$this->assertFalse( $merged['assets_touched'] );
+	}
+
+	public function test_missing_keys_in_existing_are_safe() {
+		$merged = $this->merge(
+			[ 'plugin' => 'hello' ],
+			[
+				'plugin'         => 'hello',
+				'tags_touched'   => [ 'trunk' ],
+				'revisions'      => [ 42 ],
+				'readme_touched' => true,
+			]
+		);
+
+		$this->assertSame( [ 'trunk' ], $merged['tags_touched'] );
+		$this->assertSame( [ 42 ], $merged['revisions'] );
+		$this->assertSame( [], $merged['tags_deleted'] );
+		$this->assertTrue( $merged['readme_touched'] );
+	}
+
+	/**
+	 * A commit that touches only /trunk is the candidate for the grace window.
+	 */
+	public function test_trunk_only_commit_is_classified_as_trunk_only() {
+		$this->assertTrue( $this->is_trunk_only( [
+			'tags_touched' => [ 'trunk' ],
+			'tags_deleted' => [],
+		] ) );
+	}
+
+	/**
+	 * If a tag is touched the change should never be classified as trunk-only,
+	 * even if /trunk is also in the same commit.
+	 */
+	public function test_change_touching_a_tag_is_not_trunk_only() {
+		$this->assertFalse( $this->is_trunk_only( [
+			'tags_touched' => [ 'trunk', '1.2.3' ],
+			'tags_deleted' => [],
+		] ) );
+
+		$this->assertFalse( $this->is_trunk_only( [
+			'tags_touched' => [ '1.2.3' ],
+			'tags_deleted' => [],
+		] ) );
+	}
+
+	/**
+	 * A change that deletes a tag isn't a candidate for the grace window
+	 * either: the deletion needs to propagate to the directory immediately.
+	 */
+	public function test_change_deleting_a_tag_is_not_trunk_only() {
+		$this->assertFalse( $this->is_trunk_only( [
+			'tags_touched' => [ 'trunk' ],
+			'tags_deleted' => [ '1.1.0' ],
+		] ) );
+	}
+
+	/**
+	 * An empty set of touched tags (a defensive default) shouldn't be
+	 * misclassified as a delayable trunk-only update.
+	 */
+	public function test_empty_tags_touched_is_not_trunk_only() {
+		$this->assertFalse( $this->is_trunk_only( [
+			'tags_touched' => [],
+			'tags_deleted' => [],
+		] ) );
+	}
+}