Merge pull request ampproject#1098 from Automattic/fix/0.7-vip-review

Apply tweaks from WordPress.com VIP review
jritterbush · Apr 28, 2018 · 6ff75c9 · 6ff75c9
2 parents dcb882f + 58b6f0e
commit 6ff75c9
Show file tree

Hide file tree

Showing 7 changed files with 91 additions and 80 deletions.
diff --git a/includes/options/class-amp-options-manager.php b/includes/options/class-amp-options-manager.php
@@ -30,7 +30,25 @@ public static function register_settings() {
 			)
 		);
 
-		add_action( 'update_option_' . self::OPTION_NAME, 'flush_rewrite_rules' );
+		add_action( 'update_option_' . self::OPTION_NAME, array( __CLASS__, 'maybe_flush_rewrite_rules' ), 10, 2 );
+	}
+
+	/**
+	 * Flush rewrite rules if the supported_post_types have changed.
+	 *
+	 * @since 0.6.2
+	 *
+	 * @param array $old_options Old options.
+	 * @param array $new_options New options.
+	 */
+	public static function maybe_flush_rewrite_rules( $old_options, $new_options ) {
+		$old_post_types = isset( $old_options['supported_post_types'] ) ? $old_options['supported_post_types'] : array();
+		$new_post_types = isset( $new_options['supported_post_types'] ) ? $new_options['supported_post_types'] : array();
+		sort( $old_post_types );
+		sort( $new_post_types );
+		if ( $old_post_types !== $new_post_types ) {
+			flush_rewrite_rules( false );
+		}
 	}
 
 	/**

diff --git a/includes/options/views/class-amp-analytics-options-submenu-page.php b/includes/options/views/class-amp-analytics-options-submenu-page.php
@@ -10,18 +10,23 @@
  */
 class AMP_Analytics_Options_Submenu_Page {
 
+	/**
+	 * Render entry.
+	 *
+	 * @param string $id     Entry ID.
+	 * @param string $type   Entry type.
+	 * @param string $config Entry config as serialized JSON.
+	 */
 	private function render_entry( $id = '', $type = '', $config = '' ) {
 		$is_existing_entry = ! empty( $id );
 
 		if ( $is_existing_entry ) {
-			$entry_slug = sprintf( '%s%s', ( $type ? $type . '-' : '' ), substr( $id, -6 ) );
+			$entry_slug = sprintf( '%s%s', ( $type ? $type . '-' : '' ), substr( $id, - 6 ) );
+			/* translators: %s is the entry slug */
 			$analytics_title = sprintf( __( 'Analytics: %s', 'amp' ), $entry_slug );
 		} else {
 			$analytics_title = __( 'Add new entry:', 'amp' );
-		}
-
-		if ( ! $is_existing_entry ) {
-			$id = '__new__';
+			$id              = '__new__';
 		}
 
 		$id_base = sprintf( '%s[analytics][%s]', AMP_Options_Manager::OPTION_NAME, $id );
@@ -55,9 +60,9 @@ private function render_entry( $id = '', $type = '', $config = '' ) {
 				<p>
 					<?php
 					wp_nonce_field( 'analytics-options', 'analytics-options' );
-					submit_button( __( 'Save', 'amp' ), 'primary', 'save', false );
+					submit_button( esc_html__( 'Save', 'amp' ), 'primary', 'save', false );
 					if ( $is_existing_entry ) {
-						submit_button( __( 'Delete', 'amp' ), 'delete button-primary', $id_base . '[delete]', false );
+						submit_button( esc_html__( 'Delete', 'amp' ), 'delete button-primary', esc_attr( $id_base . '[delete]' ), false );
 					}
 					?>
 				</p>
@@ -78,6 +83,9 @@ public function render_title() {
 		<?php
 	}
 
+	/**
+	 * Render.
+	 */
 	public function render() {
 		$analytics_entries = AMP_Options_Manager::get_option( 'analytics', array() );
 

diff --git a/includes/templates/class-amp-post-template.php b/includes/templates/class-amp-post-template.php
@@ -94,7 +94,13 @@ public function __construct( $post ) {
 		} else {
 			$this->post = get_post( $post );
 		}
-		$this->ID = $this->post->ID;
+
+		// Make sure we have a post, or bail if not.
+		if ( is_a( $this->post, 'WP_Post' ) ) {
+			$this->ID = $this->post->ID;
+		} else {
+			return;
+		}
 
 		$content_max_width = self::CONTENT_MAX_WIDTH;
 		if ( isset( $GLOBALS['content_width'] ) && $GLOBALS['content_width'] > 0 ) {

diff --git a/readme.md b/readme.md
@@ -8,7 +8,7 @@ Enable Accelerated Mobile Pages (AMP) on your WordPress site.
 **Tags:** [amp](https://wordpress.org/plugins/tags/amp), [mobile](https://wordpress.org/plugins/tags/mobile)  
 **Requires at least:** 4.7  
 **Tested up to:** 4.9  
-**Stable tag:** 0.6.0  
+**Stable tag:** 0.6.2  
 **License:** [GPLv2 or later](http://www.gnu.org/licenses/gpl-2.0.html)  
 **Requires PHP:** 5.3  
 

diff --git a/readme.txt b/readme.txt
@@ -3,7 +3,7 @@ Contributors: batmoo, joen, automattic, potatomaster, albertomedina, google, xwp
 Tags: amp, mobile
 Requires at least: 4.7
 Tested up to: 4.9
-Stable tag: 0.6.0
+Stable tag: 0.6.2
 License: GPLv2 or later
 License URI: http://www.gnu.org/licenses/gpl-2.0.html
 Requires PHP: 5.3

diff --git a/tests/amp-tag-and-attribute-sanitizer-private-methods-tests.php b/tests/amp-tag-and-attribute-sanitizer-private-methods-tests.php
@@ -409,12 +409,7 @@ public function test_validate_attr_spec_rules( $data, $expected ) {
 
 		$got = $this->invoke_method( $sanitizer, $data['func_name'], array( $node, $data['attribute_name'], $attr_spec_rule ) );
 
-		if ( $expected != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $source );
-			var_dump( $data );
-		}
-
-		$this->assertEquals( $expected, $got );
+		$this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $source, wp_json_encode( $data ) ) );
 	}
 
 	public function get_is_allowed_attribute_data() {
@@ -566,12 +561,7 @@ public function test_is_allowed_attribute( $data, $expected ) {
 
 		$got = $this->invoke_method( $sanitizer, $data['func_name'], array( $data['attribute_name'], $attr_spec_list ) );
 
-		if ( $expected != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $source );
-			var_dump( $data );
-		}
-
-		$this->assertEquals( $expected, $got );
+		$this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $source, wp_json_encode( $data ) ) );
 	}
 
 	public function get_remove_node_data() {
@@ -662,13 +652,7 @@ public function test_remove_node( $data, $expected ) {
 
 		$got = AMP_DOM_Utils::get_content_from_dom( $dom );
 
-		if ( $expected != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $data['source'] );
-			var_dump( $data );
-			printf( 'got = %s' .PHP_EOL, $got );
-		}
-
-		$this->assertEquals( $expected, $got );
+		$this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) );
 	}
 
 
@@ -803,13 +787,7 @@ public function test_replace_node_with_children( $data, $expected ) {
 		$got = AMP_DOM_Utils::get_content_from_dom( $dom );
 		$got = preg_replace( '/(?<=>)\s+(?=<)/', '', $got );
 
-		if ( $expected != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $data['source'] );
-			var_dump( $data );
-			printf( 'got = %s' .PHP_EOL, $got );
-		}
-
-		$this->assertEquals( $expected, $got );
+		$this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) );
 	}
 
 	public function get_ancestor_with_tag_name_data() {
@@ -865,12 +843,7 @@ public function test_get_ancestor_with_tag_name( $data, $expected ) {
 
 		$got = $this->invoke_method( $sanitizer, 'get_ancestor_with_tag_name', array( $node, $data['ancestor_tag_name'] ) );
 
-		if ( $ancestor_node != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $data['source'] );
-			var_dump( $data );
-		}
-
-		$this->assertEquals( $ancestor_node, $got );
+		$this->assertEquals( $ancestor_node, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) );
 	}
 
 	public function get_validate_attr_spec_list_for_node_data() {
@@ -1071,12 +1044,7 @@ public function test_validate_attr_spec_list_for_node( $data, $expected ) {
 
 		$got = $this->invoke_method( $sanitizer, 'validate_attr_spec_list_for_node', array( $node, $data['attr_spec_list'] ) );
 
-		if ( $expected != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $data['source'] );
-			var_dump( $data );
-		}
-
-		$this->assertEquals( $expected, $got );
+		$this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) );
 	}
 
 	public function get_check_attr_spec_rule_value_data() {
@@ -1198,12 +1166,7 @@ public function test_check_attr_spec_rule_value( $data, $expected ) {
 
 		$got = $this->invoke_method( $sanitizer, 'check_attr_spec_rule_value', array( $node, $data['attr_name'], $data['attr_spec_rule'] ) );
 
-		if ( $expected != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $data['source'] );
-			var_dump( $data );
-		}
-
-		$this->assertEquals( $expected, $got );
+		$this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) );
 	}
 
 	public function get_check_attr_spec_rule_value_casei_data() {
@@ -1350,12 +1313,7 @@ public function test_check_attr_spec_rule_value_casei( $data, $expected ) {
 
 		$got = $this->invoke_method( $sanitizer, 'check_attr_spec_rule_value_casei', array( $node, $data['attr_name'], $data['attr_spec_rule'] ) );
 
-		if ( $expected != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $data['source'] );
-			var_dump( $data );
-		}
-
-		$this->assertEquals( $expected, $got );
+		$this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) );
 	}
 
 	public function get_check_attr_spec_rule_blacklisted_value_regex() {
@@ -1444,12 +1402,7 @@ public function test_check_attr_spec_rule_blacklisted_value_regex( $data, $expec
 
 		$got = $this->invoke_method( $sanitizer, 'check_attr_spec_rule_blacklisted_value_regex', array( $node, $data['attr_name'], $data['attr_spec_rule'] ) );
 
-		if ( $expected != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $data['source'] );
-			var_dump( $data );
-		}
-
-		$this->assertEquals( $expected, $got );
+		$this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) );
 	}
 
 	public function get_check_attr_spec_rule_allowed_protocol() {
@@ -1579,12 +1532,7 @@ public function test_check_attr_spec_rule_allowed_protocol( $data, $expected ) {
 
 		$got = $this->invoke_method( $sanitizer, 'check_attr_spec_rule_allowed_protocol', array( $node, $data['attr_name'], $data['attr_spec_rule'] ) );
 
-		if ( $expected != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $data['source'] );
-			var_dump( $data );
-		}
-
-		$this->assertEquals( $expected, $got );
+		$this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) );
 	}
 
 	public function get_check_attr_spec_rule_disallowed_relative() {
@@ -1728,12 +1676,7 @@ public function test_check_attr_spec_rule_disallowed_relative( $data, $expected
 
 		$got = $this->invoke_method( $sanitizer, 'check_attr_spec_rule_disallowed_relative', array( $node, $data['attr_name'], $data['attr_spec_rule'] ) );
 
-		if ( $expected != $got ) {
-			printf( 'using source: %s' . PHP_EOL, $data['source'] );
-			var_dump( $data );
-		}
-
-		$this->assertEquals( $expected, $got );
+		$this->assertEquals( $expected, $got, sprintf( "using source: %s\n%s", $data['source'], wp_json_encode( $data ) ) );
 	}
 
 	/**

diff --git a/tests/test-class-amp-options-manager.php b/tests/test-class-amp-options-manager.php
@@ -38,7 +38,43 @@ public function test_register_settings() {
 		$this->assertArrayHasKey( AMP_Options_Manager::OPTION_NAME, $registered_settings );
 		$this->assertEquals( 'array', $registered_settings[ AMP_Options_Manager::OPTION_NAME ]['type'] );
 
-		$this->assertEquals( 10, has_action( 'update_option_' . AMP_Options_Manager::OPTION_NAME, 'flush_rewrite_rules' ) );
+		$this->assertEquals( 10, has_action( 'update_option_' . AMP_Options_Manager::OPTION_NAME, array( 'AMP_Options_Manager', 'maybe_flush_rewrite_rules' ) ) );
+	}
+
+	/**
+	 * Test maybe_flush_rewrite_rules.
+	 *
+	 * @covers AMP_Options_Manager::maybe_flush_rewrite_rules()
+	 */
+	public function test_maybe_flush_rewrite_rules() {
+		global $wp_rewrite;
+		$wp_rewrite->init();
+		AMP_Options_Manager::register_settings();
+		$dummy_rewrite_rules = array( 'previous' => true );
+
+		// Check change to supported_post_types.
+		update_option( 'rewrite_rules', $dummy_rewrite_rules );
+		AMP_Options_Manager::maybe_flush_rewrite_rules(
+			array( 'supported_post_types' => array( 'page' ) ),
+			array()
+		);
+		$this->assertEmpty( get_option( 'rewrite_rules' ) );
+
+		// Check update of supported_post_types but no change.
+		update_option( 'rewrite_rules', $dummy_rewrite_rules );
+		update_option( AMP_Options_Manager::OPTION_NAME, array(
+			array( 'supported_post_types' => array( 'page' ) ),
+			array( 'supported_post_types' => array( 'page' ) ),
+		) );
+		$this->assertEquals( $dummy_rewrite_rules, get_option( 'rewrite_rules' ) );
+
+		// Check changing a different property.
+		update_option( 'rewrite_rules', array( 'previous' => true ) );
+		update_option( AMP_Options_Manager::OPTION_NAME, array(
+			array( 'foo' => 'new' ),
+			array( 'foo' => 'old' ),
+		) );
+		$this->assertEquals( $dummy_rewrite_rules, get_option( 'rewrite_rules' ) );
 	}
 
 	/**