From 933a090093dec8b317511f2f5ab82beb305f2d8d Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Thu, 10 Sep 2020 12:16:24 +0200 Subject: [PATCH 01/11] Move block_classes method to new Blocks package. --- packages/blocks/src/class-blocks.php | 40 ++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/packages/blocks/src/class-blocks.php b/packages/blocks/src/class-blocks.php index 208707af45008..6ad5fa8968881 100644 --- a/packages/blocks/src/class-blocks.php +++ b/packages/blocks/src/class-blocks.php @@ -17,7 +17,47 @@ * @since 9.0.0 */ class Blocks { + /** + * Get CSS classes for a block. + * + * @since 9.0.0 + * + * @param string $slug Block slug. + * @param array $attr Block attributes. + * @param array $extra Potential extra classes you may want to provide. + * + * @return string $classes List of CSS classes for a block. + */ + public static function classes( $slug = '', $attr, $extra = array() ) { + if ( empty( $slug ) ) { + return ''; + } + // Basic block name class. + $classes = array( + 'wp-block-jetpack-' . $slug, + ); + + // Add alignment if provided. + if ( + ! empty( $attr['align'] ) + && in_array( $attr['align'], array( 'left', 'center', 'right', 'wide', 'full' ), true ) + ) { + array_push( $classes, 'align' . $attr['align'] ); + } + + // Add custom classes if provided in the block editor. + if ( ! empty( $attr['className'] ) ) { + array_push( $classes, $attr['className'] ); + } + + // Add any extra classes. + if ( is_array( $extra ) && ! empty( $extra ) ) { + $classes = array_merge( $classes, array_filter( $extra ) ); + } + + return implode( ' ', $classes ); + } } From 32f25d8055ec89b7c293d8ab22fd0fd8ff20670e Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Tue, 8 Sep 2020 17:29:27 +0200 Subject: [PATCH 02/11] Add new method to check for AMP view from the package. --- packages/blocks/src/class-blocks.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/blocks/src/class-blocks.php b/packages/blocks/src/class-blocks.php index 6ad5fa8968881..bbd9ea6edb234 100644 --- a/packages/blocks/src/class-blocks.php +++ b/packages/blocks/src/class-blocks.php @@ -58,6 +58,18 @@ public static function classes( $slug = '', $attr, $extra = array() ) { return implode( ' ', $classes ); } + + /** + * Does the page return AMP content. + * + * @return bool $is_amp_request Are we on am AMP view. + */ + public static function is_amp_request() { + $is_amp_request = ( function_exists( 'is_amp_endpoint' ) && is_amp_endpoint() ); + + /** This filter is documented in 3rd-party/class.jetpack-amp-support.php */ + return apply_filters( 'jetpack_is_amp_request', $is_amp_request ); + } } From 7c1142b45d996b47c8fc6745b81bce9cbf8bb98e Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Thu, 10 Sep 2020 13:19:09 +0200 Subject: [PATCH 03/11] Add tests for new methods --- packages/blocks/tests/php/bootstrap.php | 16 ++++++ packages/blocks/tests/php/test-blocks.php | 69 +++++++++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 packages/blocks/tests/php/bootstrap.php create mode 100644 packages/blocks/tests/php/test-blocks.php diff --git a/packages/blocks/tests/php/bootstrap.php b/packages/blocks/tests/php/bootstrap.php new file mode 100644 index 0000000000000..9a50b9a7a6730 --- /dev/null +++ b/packages/blocks/tests/php/bootstrap.php @@ -0,0 +1,16 @@ + 'baz', + 'align' => 'wide', + 'className' => 'editorclass', + ); + $extra = array( 'extraclass' ); + + $block_classes = Blocks::classes( $block_name, $attr, $extra ); + + $this->assertContains( 'wp-block-jetpack-foo', $block_classes ); // a general class is created from the block name. + $this->assertNotContains( 'bar', $block_classes ); // The extra 'foo' attribute should be dropped. + $this->assertContains( 'alignwide', $block_classes ); // an alignment class is created. + $this->assertContains( 'editorclass', $block_classes ); // className classes are passed. + $this->assertContains( 'extraclass', $block_classes ); // Extra class remains. + } + + /** + * Test whether we can detect an AMP view. + * + * @since 9.0.0 + * + * @covers Automattic\Jetpack\Blocks::is_amp_request + */ + public function test_is_amp_request() { + add_filter( 'jetpack_is_amp_request', '__return_true' ); + + $this->assertTrue( Blocks::is_amp_request() ); + + remove_filter( 'jetpack_is_amp_request', '__return_true' ); + } + + /** + * Test whether we can detect an AMP view. + * + * @since 9.0.0 + * + * @covers Automattic\Jetpack\Blocks::is_amp_request + */ + public function test_is_not_amp_request() { + $this->assertFalse( Blocks::is_amp_request() ); + } +} From 785c7a629cde07ace6040804a828164b5c2f17b7 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Tue, 15 Sep 2020 16:21:52 +0200 Subject: [PATCH 04/11] Move away from array_push See https://github.com/Automattic/jetpack/pull/17126#discussion_r488058806 --- packages/blocks/src/class-blocks.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/blocks/src/class-blocks.php b/packages/blocks/src/class-blocks.php index bbd9ea6edb234..bd1834c246239 100644 --- a/packages/blocks/src/class-blocks.php +++ b/packages/blocks/src/class-blocks.php @@ -43,12 +43,12 @@ public static function classes( $slug = '', $attr, $extra = array() ) { ! empty( $attr['align'] ) && in_array( $attr['align'], array( 'left', 'center', 'right', 'wide', 'full' ), true ) ) { - array_push( $classes, 'align' . $attr['align'] ); + $classes[] = 'align' . $attr['align']; } // Add custom classes if provided in the block editor. if ( ! empty( $attr['className'] ) ) { - array_push( $classes, $attr['className'] ); + $classes[] = $attr['className']; } // Add any extra classes. From 0e8b450fc52cd5d8945c4984e4280b4be79584ba Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Tue, 15 Sep 2020 16:24:48 +0200 Subject: [PATCH 05/11] Fix typo Co-authored-by: Brad Jorsch --- packages/blocks/src/class-blocks.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/blocks/src/class-blocks.php b/packages/blocks/src/class-blocks.php index bd1834c246239..5ac700fa3b0c7 100644 --- a/packages/blocks/src/class-blocks.php +++ b/packages/blocks/src/class-blocks.php @@ -62,7 +62,7 @@ public static function classes( $slug = '', $attr, $extra = array() ) { /** * Does the page return AMP content. * - * @return bool $is_amp_request Are we on am AMP view. + * @return bool $is_amp_request Are we on an AMP view. */ public static function is_amp_request() { $is_amp_request = ( function_exists( 'is_amp_endpoint' ) && is_amp_endpoint() ); @@ -72,4 +72,3 @@ public static function is_amp_request() { } } - From 826249cd8cc15c1ec326cfa6ddc8ddff6eab6347 Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Tue, 15 Sep 2020 16:26:22 +0200 Subject: [PATCH 06/11] Update test with proper bar and baz attributes See https://github.com/Automattic/jetpack/pull/17126#discussion_r488068984 --- packages/blocks/tests/php/test-blocks.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/blocks/tests/php/test-blocks.php b/packages/blocks/tests/php/test-blocks.php index b6cf47243de7e..88b5b6942771d 100644 --- a/packages/blocks/tests/php/test-blocks.php +++ b/packages/blocks/tests/php/test-blocks.php @@ -35,7 +35,8 @@ public function test_block_classes() { $block_classes = Blocks::classes( $block_name, $attr, $extra ); $this->assertContains( 'wp-block-jetpack-foo', $block_classes ); // a general class is created from the block name. - $this->assertNotContains( 'bar', $block_classes ); // The extra 'foo' attribute should be dropped. + $this->assertNotContains( 'bar', $block_classes ); // The extra 'bar' attribute should be dropped. + $this->assertNotContains( 'baz', $block_classes ); // The extra 'baz' attribute should be dropped. $this->assertContains( 'alignwide', $block_classes ); // an alignment class is created. $this->assertContains( 'editorclass', $block_classes ); // className classes are passed. $this->assertContains( 'extraclass', $block_classes ); // Extra class remains. From 05858ecbdab5d1c9c7f5475057016de6b3f5dcd1 Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Tue, 15 Sep 2020 16:29:53 +0200 Subject: [PATCH 07/11] Add more assertions to cover all attributes See https://github.com/Automattic/jetpack/pull/17126#discussion_r488069609 --- packages/blocks/tests/php/test-blocks.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/blocks/tests/php/test-blocks.php b/packages/blocks/tests/php/test-blocks.php index 88b5b6942771d..65b9a61aa35e1 100644 --- a/packages/blocks/tests/php/test-blocks.php +++ b/packages/blocks/tests/php/test-blocks.php @@ -37,6 +37,8 @@ public function test_block_classes() { $this->assertContains( 'wp-block-jetpack-foo', $block_classes ); // a general class is created from the block name. $this->assertNotContains( 'bar', $block_classes ); // The extra 'bar' attribute should be dropped. $this->assertNotContains( 'baz', $block_classes ); // The extra 'baz' attribute should be dropped. + $this->assertNotContains( 'align ', $block_classes ); // The align attribute should only be used to create a new attribute. + $this->assertNotContains( 'className', $block_classes ); // The className attribute should be dropped, only the editorclass value should remain. $this->assertContains( 'alignwide', $block_classes ); // an alignment class is created. $this->assertContains( 'editorclass', $block_classes ); // className classes are passed. $this->assertContains( 'extraclass', $block_classes ); // Extra class remains. From d9805e923c6e3b4b33fba812ab7527dc51c77c9e Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Tue, 15 Sep 2020 16:33:13 +0200 Subject: [PATCH 08/11] Add test for invalid alignment value. See https://github.com/Automattic/jetpack/pull/17126#discussion_r488069609 --- packages/blocks/tests/php/test-blocks.php | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/packages/blocks/tests/php/test-blocks.php b/packages/blocks/tests/php/test-blocks.php index 65b9a61aa35e1..e2aef952f653a 100644 --- a/packages/blocks/tests/php/test-blocks.php +++ b/packages/blocks/tests/php/test-blocks.php @@ -44,6 +44,20 @@ public function test_block_classes() { $this->assertContains( 'extraclass', $block_classes ); // Extra class remains. } + /** + * Test for invalid alignment values. + * + * @since 9.0.0 + * + * @covers Automattic\Jetpack\Blocks::classes + */ + public function test_block_classes_invalid_align() { + $attr = array( 'align' => 'test' ); + $block_classes = Blocks::classes( 'test', $attr ); + + $this->assertNotContains( 'aligntest', $block_classes ); + } + /** * Test whether we can detect an AMP view. * From e1d868acba6f7dca95970a20a1091022a069acd7 Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Tue, 15 Sep 2020 16:37:24 +0200 Subject: [PATCH 09/11] Ensure we always remove AMP filter, even when test fails. See https://github.com/Automattic/jetpack/pull/17126#discussion_r488070946 --- packages/blocks/tests/php/test-blocks.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/blocks/tests/php/test-blocks.php b/packages/blocks/tests/php/test-blocks.php index e2aef952f653a..940be5034aa4f 100644 --- a/packages/blocks/tests/php/test-blocks.php +++ b/packages/blocks/tests/php/test-blocks.php @@ -67,10 +67,10 @@ public function test_block_classes_invalid_align() { */ public function test_is_amp_request() { add_filter( 'jetpack_is_amp_request', '__return_true' ); - - $this->assertTrue( Blocks::is_amp_request() ); - + $is_amp = Blocks::is_amp_request(); remove_filter( 'jetpack_is_amp_request', '__return_true' ); + + $this->assertTrue( $is_amp ); } /** From 83fb164cbbe73dd65dabc88851126b3a0287fbdd Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Tue, 15 Sep 2020 16:42:29 +0200 Subject: [PATCH 10/11] Remove unneeded default See https://github.com/Automattic/jetpack/pull/17126#discussion_r488057122 --- packages/blocks/src/class-blocks.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/blocks/src/class-blocks.php b/packages/blocks/src/class-blocks.php index 5ac700fa3b0c7..69b0d07b11172 100644 --- a/packages/blocks/src/class-blocks.php +++ b/packages/blocks/src/class-blocks.php @@ -28,7 +28,7 @@ class Blocks { * * @return string $classes List of CSS classes for a block. */ - public static function classes( $slug = '', $attr, $extra = array() ) { + public static function classes( $slug, $attr, $extra = array() ) { if ( empty( $slug ) ) { return ''; } From d56a0a906916541bdf009a92da8e4cb7749ba6f1 Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Tue, 15 Sep 2020 18:10:03 +0200 Subject: [PATCH 11/11] Switch to alternative approach to handle potential test failures See https://github.com/Automattic/jetpack/pull/17126/files#r488758175 --- packages/blocks/tests/php/test-blocks.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/blocks/tests/php/test-blocks.php b/packages/blocks/tests/php/test-blocks.php index 940be5034aa4f..464012699cc20 100644 --- a/packages/blocks/tests/php/test-blocks.php +++ b/packages/blocks/tests/php/test-blocks.php @@ -67,10 +67,11 @@ public function test_block_classes_invalid_align() { */ public function test_is_amp_request() { add_filter( 'jetpack_is_amp_request', '__return_true' ); - $is_amp = Blocks::is_amp_request(); - remove_filter( 'jetpack_is_amp_request', '__return_true' ); - - $this->assertTrue( $is_amp ); + try { + $this->assertTrue( Blocks::is_amp_request() ); + } finally { + remove_filter( 'jetpack_is_amp_request', '__return_true' ); + } } /**