Merge pull request #32540 from woocommerce/fix/errors-flash-messaging

Reduce risk of prematurely dropping meta box error messages

plugins/woocommerce/changelog/fix-errors-flash-messaging

@@ -0,0 +1,4 @@
+Significance: minor
+Type: tweak
+
+Changes to reduce the risk of admin error messages being discarded prematurely.

plugins/woocommerce/includes/admin/class-wc-admin-meta-boxes.php

@@ -15,6 +15,12 @@ defined( 'ABSPATH' ) || exit;
  * WC_Admin_Meta_Boxes.
  */
 class WC_Admin_Meta_Boxes {
+	/**
+	 * Name of the option used to store errors to be displayed at the next suitable opportunity.
+	 *
+	 * @since 6.5.0
+	 */
+	public const ERROR_STORE = 'woocommerce_meta_box_errors';
 
 	/**
 	 * Is meta boxes saved once?
@@ -66,7 +72,7 @@ class WC_Admin_Meta_Boxes {
 
 		// Error handling (for showing errors from meta boxes on next page load).
 		add_action( 'admin_notices', array( $this, 'output_errors' ) );
-		add_action( 'shutdown', array( $this, 'save_errors' ) );
+		add_action( 'shutdown', array( $this, 'append_to_error_store' ) );
 
 		add_filter( 'theme_product_templates', array( $this, 'remove_block_templates' ), 10, 1 );
 	}
@@ -82,16 +88,34 @@ class WC_Admin_Meta_Boxes {
 
 	/**
 	 * Save errors to an option.
+	 *
+	 * Note that calling this will overwrite any errors that have already been stored via the Options API.
+	 * Unless you are sure you want this, consider using the append_to_error_store() method instead.
 	 */
 	public function save_errors() {
-		update_option( 'woocommerce_meta_box_errors', self::$meta_box_errors );
+		update_option( self::ERROR_STORE, self::$meta_box_errors );
+	}
+
+	/**
+	 * If additional errors have been added in the current request (ie, via the add_error() method) then they
+	 * will be added to the persistent error store via the Options API.
+	 *
+	 * @since 6.5.0
+	 */
+	public function append_to_error_store() {
+		if ( empty( self::$meta_box_errors ) ) {
+			return;
+		}
+
+		$existing_errors = get_option( self::ERROR_STORE, array() );
+		update_option( self::ERROR_STORE, array_unique( array_merge( $existing_errors, self::$meta_box_errors ) ) );
 	}
 
 	/**
 	 * Show any stored error messages.
 	 */
 	public function output_errors() {
-		$errors = array_filter( (array) get_option( 'woocommerce_meta_box_errors' ) );
+		$errors = array_filter( (array) get_option( self::ERROR_STORE ) );
 
 		if ( ! empty( $errors ) ) {
 
@@ -104,7 +128,7 @@ class WC_Admin_Meta_Boxes {
 			echo '</div>';
 
 			// Clear.
-			delete_option( 'woocommerce_meta_box_errors' );
+			delete_option( self::ERROR_STORE );
 		}
 	}
 

plugins/woocommerce/includes/class-wc-ajax.php

@@ -2226,7 +2226,7 @@ class WC_AJAX {
 			echo '<button type="button" class="notice-dismiss"><span class="screen-reader-text">' . esc_html__( 'Dismiss this notice.', 'woocommerce' ) . '</span></button>';
 			echo '</div>';
 
-			delete_option( 'woocommerce_meta_box_errors' );
+			delete_option( WC_Admin_Meta_Boxes::ERROR_STORE );
 		}
 
 		wp_die();
@@ -3095,7 +3095,7 @@ class WC_AJAX {
 				}
 				$enabled = $gateway->get_option( 'enabled', 'no' );
 				$option  = array(
-					'id' => $gateway->get_option_key()
+					'id' => $gateway->get_option_key(),
 				);
 
 				if ( ! wc_string_to_bool( $enabled ) ) {

plugins/woocommerce/tests/php/includes/admin/class-wc-admin-meta-boxes-test.php

@@ -0,0 +1,73 @@
+<?php
+
+/**
+ * Tests for meta box-related functionality in the product editor.
+ */
+class WC_Admin_Meta_Boxes_Test extends WC_Unit_Test_Case {
+	/**
+	 * @var WC_Admin_Meta_Boxes
+	 */
+	private $sut;
+
+	/**
+	 * Create subject-under-test.
+	 */
+	public function set_up() {
+		$this->sut = new WC_Admin_Meta_Boxes();
+		parent::set_up();
+	}
+
+	/**
+	 * @testdox Test that meta box errors can be stored and retrieved as expected.
+	 */
+	public function test_persistence_of_meta_box_errors() {
+		WC_Admin_Meta_Boxes::add_error( 'Oh no!' );
+		WC_Admin_Meta_Boxes::add_error( 'Crikey!' );
+
+		$error_output = $this->get_meta_box_error_output();
+		$this->assertEmpty( $error_output, 'If the errors have not first been saved to the database, they cannot be retrieved for display.' );
+
+		$this->simulate_shutdown();
+		$error_output = $this->get_meta_box_error_output();
+		$this->assertStringContainsString( 'Oh no!', $error_output, 'The error output contains the expected error string (test #1).' );
+		$this->assertStringContainsString( 'Crikey!', $error_output, 'The error output contains the expected error string (test #2).' );
+
+		$error_output = $this->get_meta_box_error_output();
+		$this->assertEmpty( $error_output, 'The error store is cleared after errors have been output.' );
+	}
+
+	/**
+	 * @testdox Test that the stored meta box errors are not accidentally cleared by concurrent requests before they are rendered.
+	 */
+	public function test_meta_box_errors_are_not_accidentally_cleared_during_shutdown() {
+		WC_Admin_Meta_Boxes::add_error( 'Yikes!' );
+
+		$this->simulate_shutdown();
+		$this->simulate_shutdown();
+
+		$error_output = $this->get_meta_box_error_output();
+		$this->assertStringContainsString( 'Yikes!', $error_output, 'The stored error persisted across requests.' );
+	}
+
+	/**
+	 * Calls the WC_Admin_Meta_Boxes::output_errors() method, capturing and returning the output.
+	 *
+	 * @return string
+	 */
+	private function get_meta_box_error_output(): string {
+		ob_start();
+		$this->sut->output_errors();
+		return ob_get_clean();
+	}
+
+	/**
+	 * Simulates what normally happens when `shutdown` occurs, in relation to the WC_Admin_Meta_Boxes class.
+	 * We avoid actually calling `do_action( 'shutdown' )` because we do not have perfect isolation between tests, and
+	 * wish to avoid unwanted side-effects unrelated to this set of tests.
+	 */
+	private function simulate_shutdown() {
+		// Previously (prior to 6.5.0), $this->sut->save_errors() would have been called during shutdown.
+		$this->sut->append_to_error_store();
+		WC_Admin_Meta_Boxes::$meta_box_errors = array();
+	}
+}