Eliminate examination of collected validation errors to decide to omi…

…t amp-form script
ampproject · Aug 10, 2021 · 531de25 · 531de25
1 parent 9fd8185
commit 531de25
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 11 deletions.
diff --git a/includes/class-amp-theme-support.php b/includes/class-amp-theme-support.php
@@ -1514,17 +1514,8 @@ public static function ensure_required_markup( Document $dom, $script_handles =
 		}
 
 		// When opting-in to POST forms, omit the amp-form component entirely since it blocks submission.
-		if ( amp_is_allowing_post_forms() ) {
-			foreach ( AMP_Validation_Manager::$validation_results as $validation_result ) {
-				if (
-					! $validation_result['sanitized']
-					&&
-					AMP_Form_Sanitizer::FORM_HAS_POST_METHOD === $validation_result['error']['code']
-				) {
-					$superfluous_script_handles[] = Extension::FORM;
-					break;
-				}
-			}
+		if ( amp_is_allowing_post_forms() && $dom->xpath->query( '//form[ @method = "post" and @action ]' )->length > 0 ) {
+			$superfluous_script_handles[] = Extension::FORM;
 		}
 
 		foreach ( $superfluous_script_handles as $superfluous_script_handle ) {

diff --git a/tests/php/test-class-amp-theme-support.php b/tests/php/test-class-amp-theme-support.php
@@ -14,7 +14,10 @@
 use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility;
 use AmpProject\AmpWP\Tests\Helpers\PrivateAccess;
 use AmpProject\AmpWP\Tests\Helpers\LoadsCoreThemes;
+use AmpProject\Attribute;
+use AmpProject\DevMode;
 use AmpProject\Dom\Document;
+use AmpProject\Dom\Element;
 use org\bovigo\vfs;
 
 /**
@@ -1796,6 +1799,68 @@ public function test_prepare_response_standard_mode_non_amp() {
 		$this->assertStringContains( 'document.write = function', $sanitized_html, 'Override of document.write() is present.' );
 	}
 
+	/** @return array */
+	public function get_data_for_allowing_post_forms() {
+		return [
+			'keep_post_forms'    => [ false ],
+			'convert_post_forms' => [ true ],
+		];
+	}
+
+	/**
+	 * Test prepare_response when allowing post forms.
+	 *
+	 * @dataProvider get_data_for_allowing_post_forms
+	 * @covers AMP_Theme_Support::prepare_response()
+	 * @param bool $converted Whether the POST form should be converted to amp-form.
+	 */
+	public function test_prepare_response_when_allowing_post_forms( $converted ) {
+		$this->set_template_mode( AMP_Theme_Support::STANDARD_MODE_SLUG );
+
+		add_filter(
+			'amp_validation_error_default_sanitized',
+			static function ( $sanitized, $error ) use ( $converted ) {
+				if ( AMP_Form_Sanitizer::FORM_HAS_POST_METHOD === $error['code'] ) {
+					$sanitized = $converted;
+				}
+				return $sanitized;
+			},
+			10,
+			2
+		);
+
+		wp();
+		add_filter( 'amp_allowing_native_post_forms', '__return_true' );
+		AMP_Theme_Support::init();
+		AMP_Theme_Support::finish_init();
+		ob_start();
+		?>
+		<html amp>
+			<body>
+				<form action="https://example.com/" method="post">
+					<button type="submit">Submit!</button>
+				</form>
+			</body>
+		</html>
+		<?php
+		$html = AMP_Theme_Support::prepare_response( ob_get_clean() );
+
+		$dom = Document::fromHtml( $html );
+
+		$this->assertEquals( $converted, $dom->documentElement->hasAttribute( Attribute::AMP ) );
+		$this->assertEquals( ! $converted, $dom->documentElement->hasAttribute( DevMode::DEV_MODE_ATTRIBUTE ) );
+		$this->assertEquals(
+			$converted,
+			$dom->xpath->query( '//script[ @custom-element = "amp-form" ]' )->length > 0
+		);
+		$form = $dom->getElementsByTagName( 'form' )->item( 0 );
+		$this->assertInstanceOf( Element::class, $form );
+		$this->assertEquals( 'post', $form->getAttribute( Attribute::METHOD ) );
+		$this->assertEquals( $converted, $form->hasAttribute( Attribute::ACTION_XHR ) );
+		$this->assertEquals( ! $converted, $form->hasAttribute( Attribute::ACTION ) );
+		$this->assertEquals( ! $converted, $form->hasAttribute( DevMode::DEV_MODE_ATTRIBUTE ) );
+	}
+
 	/**
 	 * Test prepare_response when submitting form.
 	 *