Update embeds handlers to unwrap embeds from <p> tag
#7564
Conversation
thelovekesh
force-pushed
the
fix/wrapped-embeds-in-p-tags
branch
from
97c0df5 to
ca161e5
Compare
| @@ -360,7 +360,7 @@ jobs: | |||
| WP_CORE_DIR: /tmp/wordpress | |||
| services: | |||
| mysql: | |||
| image: mariadb:latest | |||
| image: mariadb:10 | |||
There was a problem hiding this comment.
Note
The most recent MariaDB images appear to have a problem that prevents DB services from starting up. Once this is rectified in a future release, this change may be undone. In the meantime, mearidb:10 will work for us going forward.
There was a problem hiding this comment.
@thelovekesh Here's a more future-proof workaround: swissspidy/preferred-languages@ce3f0d5
There was a problem hiding this comment.
Thanks @swissspidy. This will be updated as a part of #7560.
thelovekesh
force-pushed
the
fix/wrapped-embeds-in-p-tags
branch
from
9f5d3fc to
775b6fa
Compare
thelovekesh
force-pushed
the
fix/wrapped-embeds-in-p-tags
branch
from
775b6fa to
8b8042a
Compare
| @@ -39,7 +39,7 @@ public function unregister_embed() { | |||
| */ | |||
| public function filter_embed_oembed_html( $cache, $url, $attr ) { | |||
| $parsed_url = wp_parse_url( $url ); | |||
| if ( empty( $parsed_url['host'] ) || empty( $parsed_url['path'] ) || ! preg_match( '#(^|\.)(?P<host>polldaddy\.com|crowdsignal\.com|survey\.fm|poll\.fm)#', $parsed_url['host'], $matches ) ) { | |||
| if ( empty( $parsed_url['host'] ) || empty( $parsed_url['path'] ) || ! preg_match( '#(^|\.)(?P<host>polldaddy\.com|poll\.fm|crowdsignal\.com|survey\.fm|poll\.fm)#', $parsed_url['host'], $matches ) ) { | |||
There was a problem hiding this comment.
Seems like poll.fm is now duplicated in the regex.
| protected function unwrap_p_element_by_child_tag_name( $dom, $child_tag_name ) { | ||
| $nodes = $dom->xpath->query( "//p/{$child_tag_name}" ); | ||
|
|
||
| if ( $nodes->length && $nodes instanceof DOMNodeList ) { |
There was a problem hiding this comment.
Seems like the order is backwards, and I don't think $nodes->length needs to be checked since the foreach will do that:
| if ( $nodes->length && $nodes instanceof DOMNodeList ) { | |
| if ( $nodes instanceof DOMNodeList ) { |
| */ | ||
| public function sanitize_raw_embeds( Document $dom ) { | ||
| // If there were any previous embeds in the DOM that were wrapped by `wpautop()`, unwrap them. | ||
| foreach ( [ Extension::IFRAME, Tag::IFRAME ] as $tag_name ) { |
There was a problem hiding this comment.
I'm wary of doing this for all iframes. I think it should be targeted specifically to embeds we know about. There could legitimately be a non-embed iframe which is inside of a p, and this could break styling.
Do we need AMP_Iframe_Embed_Handler?
| $this->unwrap_p_element( $embed_node ); | ||
| } | ||
| } | ||
| $this->unwrap_p_element_by_child_tag_name( $dom, Extension::FACEBOOK ); |
There was a problem hiding this comment.
Might as well reuse this constant in where $this->amp_tag is set in the class above, i.e.:
private $amp_tag = Extension::FACEBOOK;
Summary
Fixes #4450
Checklist