From ebb2a7021434f0bca1a649874215196659fbb4a4 Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Fri, 6 Sep 2019 18:48:52 +0200 Subject: [PATCH 1/5] Carousel: improve detection of containers where we should add data Fixes #13428 Until now we would rely on 2 str_replace to add Carousel data to div and ul containers in post content. This worked, but wasn't smart enough to know to ignore specific divs like Columns blocks. By using DOMDocument, we can go through the post content in a more readable way, to detect gallery blocks as well as regular galleries and images. --- modules/carousel/jetpack-carousel.php | 47 +++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/modules/carousel/jetpack-carousel.php b/modules/carousel/jetpack-carousel.php index 0bce42d91d4f3..541bc0979d34a 100644 --- a/modules/carousel/jetpack-carousel.php +++ b/modules/carousel/jetpack-carousel.php @@ -540,9 +540,52 @@ class_exists( 'Jetpack_AMP_Support' ) * @param array $extra_data Array of data about the site and the post. */ $extra_data = apply_filters( 'jp_carousel_add_data_to_container', $extra_data ); + foreach ( (array) $extra_data as $data_key => $data_values ) { - $html = str_replace( '
loadHTML( $html ); // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged + libxml_use_internal_errors( false ); + + // Let's look for lists and divs. + $ul_tags = $dom_doc->getElementsByTagName( 'ul' ); + $div_tags = $dom_doc->getElementsByTagName( 'div' ); + + // Loop through each ul, and add the data to it if it is a gallery block. + foreach ( $ul_tags as $ul_tag ) { + if ( false !== strpos( $ul_tag->getAttribute( 'class' ), 'wp-block-gallery' ) ) { + $ul_tag->setAttribute( + esc_attr( $data_key ), + wp_json_encode( $data_values ) + ); + } + } + + /* + * Loop through each div and add the data, only when it's a gallery block div. + * We want to avoid adding data to divs like wp-block-columns. + * We do however want data on divs like wp-block-jetpack-tiled-gallery. + */ + foreach ( $div_tags as $div_tag ) { + if ( + false === strpos( $div_tag->getAttribute( 'class' ), 'wp-block-' ) + || false !== strpos( $div_tag->getAttribute( 'class' ), 'gallery' ) + ) { + $div_tag->setAttribute( + esc_attr( $data_key ), + wp_json_encode( $data_values ) + ); + } + } + + // Save our updated HTML. + $html = $dom_doc->saveHTML(); } } From fef2a1a6b02b5af974adae7302306125bc34f14c Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Fri, 6 Sep 2019 19:07:39 +0200 Subject: [PATCH 2/5] Bail if the server does not support DOMDocument --- modules/carousel/jetpack-carousel.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/modules/carousel/jetpack-carousel.php b/modules/carousel/jetpack-carousel.php index 541bc0979d34a..b73ddc8e0f9d0 100644 --- a/modules/carousel/jetpack-carousel.php +++ b/modules/carousel/jetpack-carousel.php @@ -542,6 +542,11 @@ class_exists( 'Jetpack_AMP_Support' ) $extra_data = apply_filters( 'jp_carousel_add_data_to_container', $extra_data ); foreach ( (array) $extra_data as $data_key => $data_values ) { + // Do not go any further if DOMDocument is disabled on the server. + if ( ! class_exists( 'DOMDocument' ) ) { + return $html; + } + // Let's grab all containers from the HTML. $dom_doc = new DOMDocument(); From ef5cb8a49ba34a256089c5102de5e931710eb409 Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Mon, 9 Sep 2019 08:39:25 +0200 Subject: [PATCH 3/5] Store and return value from libxml_use_internal_errors See https://github.com/Automattic/jetpack/pull/13446/files#r321866447 Co-Authored-By: Michael D Adams --- modules/carousel/jetpack-carousel.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/carousel/jetpack-carousel.php b/modules/carousel/jetpack-carousel.php index b73ddc8e0f9d0..0afac9d975b9e 100644 --- a/modules/carousel/jetpack-carousel.php +++ b/modules/carousel/jetpack-carousel.php @@ -554,9 +554,9 @@ class_exists( 'Jetpack_AMP_Support' ) * The @ is not enough to suppress errors when dealing with libxml, * we have to tell it directly how we want to handle errors. */ - libxml_use_internal_errors( true ); + $old_libxml_use_internal_errors = libxml_use_internal_errors( true ); @$dom_doc->loadHTML( $html ); // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged - libxml_use_internal_errors( false ); + libxml_use_internal_errors( $old_libxml_use_internal_errors ); // Let's look for lists and divs. $ul_tags = $dom_doc->getElementsByTagName( 'ul' ); From b2a056030626f78ad7a3b3c4ae19513238b3b02b Mon Sep 17 00:00:00 2001 From: Jeremy Herve Date: Mon, 9 Sep 2019 08:44:17 +0200 Subject: [PATCH 4/5] No need to escape data key See https://github.com/Automattic/jetpack/pull/13446/files#r321867238 Co-Authored-By: Michael D Adams --- modules/carousel/jetpack-carousel.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/carousel/jetpack-carousel.php b/modules/carousel/jetpack-carousel.php index 0afac9d975b9e..ecfc32df4b8cb 100644 --- a/modules/carousel/jetpack-carousel.php +++ b/modules/carousel/jetpack-carousel.php @@ -566,7 +566,7 @@ class_exists( 'Jetpack_AMP_Support' ) foreach ( $ul_tags as $ul_tag ) { if ( false !== strpos( $ul_tag->getAttribute( 'class' ), 'wp-block-gallery' ) ) { $ul_tag->setAttribute( - esc_attr( $data_key ), + $data_key, wp_json_encode( $data_values ) ); } @@ -583,7 +583,7 @@ class_exists( 'Jetpack_AMP_Support' ) || false !== strpos( $div_tag->getAttribute( 'class' ), 'gallery' ) ) { $div_tag->setAttribute( - esc_attr( $data_key ), + $data_key, wp_json_encode( $data_values ) ); } From 6c293bbf6873365575c340b37e77ef7c2b56eb40 Mon Sep 17 00:00:00 2001 From: Michael D Adams Date: Mon, 9 Sep 2019 08:49:08 +0200 Subject: [PATCH 5/5] Wrap DOM usage in libxml_disable_entity_loader See https://github.com/Automattic/jetpack/pull/13446#pullrequestreview-285040971 --- modules/carousel/jetpack-carousel.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/carousel/jetpack-carousel.php b/modules/carousel/jetpack-carousel.php index ecfc32df4b8cb..fd3e9e4f1ad59 100644 --- a/modules/carousel/jetpack-carousel.php +++ b/modules/carousel/jetpack-carousel.php @@ -554,9 +554,11 @@ class_exists( 'Jetpack_AMP_Support' ) * The @ is not enough to suppress errors when dealing with libxml, * we have to tell it directly how we want to handle errors. */ - $old_libxml_use_internal_errors = libxml_use_internal_errors( true ); + $old_libxml_disable_entity_loader = libxml_disable_entity_loader( true ); + $old_libxml_use_internal_errors = libxml_use_internal_errors( true ); @$dom_doc->loadHTML( $html ); // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged libxml_use_internal_errors( $old_libxml_use_internal_errors ); + libxml_disable_entity_loader( $old_libxml_disable_entity_loader ); // Let's look for lists and divs. $ul_tags = $dom_doc->getElementsByTagName( 'ul' );