New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add create-embed-test-post script and fix support for various embeds #829

Merged
merged 25 commits into from Dec 14, 2017

Conversation

Projects
None yet
4 participants
@westonruter
Collaborator

westonruter commented Dec 12, 2017

Fixes #806.

'content' => function( $data ) {
return sprintf( '[gallery ids="%s"]', implode( ',', $data['ids'] ) );
},
),

This comment has been minimized.

@westonruter

westonruter Dec 12, 2017

Collaborator

This needs now to be fleshed out with all that can be thrown at content in a vanilla WordPress install.

@kienstra kienstra self-assigned this Dec 12, 2017

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Dec 12, 2017

In Development

Hi @westonruter,
Thanks a lot for starting this. I'm working on this now if that's alright.

@westonruter

This comment has been minimized.

Collaborator

westonruter commented Dec 12, 2017

Go for it!

@westonruter

This comment has been minimized.

Collaborator

westonruter commented Dec 13, 2017

As time allows, we'll also want to do something similar for Gutenberg blocks.

kienstra added some commits Dec 13, 2017

Issue 806 : Add media and embeds to test page.
Use Weston's wp-cli script to create a test post.
Add media items to the post content.
And add several embeds.
Several embeds are still not rendering,
Even on a standard, non-AMP page.
Issue 806 : Align equals signs vertically.
Before, the equals sign alignment caused a PHPCS warning.
Fix this, to enable the Travis build to succeed.
Issue 804 : Test WordPress post embeds.
create-embed-test-post.php needs to test all post content.
As Weston mentioned, embedded posts are part of this.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Dec 13, 2017

Request For Review
(For Issue #806)

Hi @ThierryA or @westonruter,
Could you please review my commits here, building on @westonruter's wp-cli file create-embed-test-post.php? To run it, simply cd to plugin's root, and run wp eval-file bin/create-embed-test-post.php.

This adds most of the allowed embeds to the post content. It also tests for other media types, as listed in Issue #806.

Also, @ThierryA's comment in #806 lists whether these newly-added embeds render properly in the AMP page.

There are some embeds that don't even render in the standard WordPress page, like Scribd and Photobucket.

If you'd like, I could work on Gutenberg block support tomorrow (Wednesday).

@kienstra kienstra requested a review from ThierryA Dec 13, 2017

@kienstra kienstra assigned ThierryA and unassigned kienstra Dec 13, 2017

@kienstra kienstra changed the title from [WIP] Add skeleton for create-embed-test-post script to Add skeleton for create-embed-test-post script Dec 13, 2017

@kienstra kienstra changed the title from Add skeleton for create-embed-test-post script to Issue 806 : Add skeleton for create-embed-test-post script Dec 13, 2017

@ThierryA

ThierryA approved these changes Dec 13, 2017 edited

Thanks @kienstra and @westonruter, great work! I pushed this commit which does the following:

  • simplify code
  • remove PHP5.2 Travis exclude as it is no longer needed
  • add errors handling

Ready for your review and adjustments if need.

PS: this PR is approved from my perspective.

@@ -35,10 +209,63 @@ function amp_test_prepare_image_attachments( $data ) {
) );
$data['ids'] = wp_list_pluck( $attachments, 'ID' );
// @todo Add some attachments if count( $data['ids'] ) < 5.
$image_count_needed = 5;

This comment has been minimized.

@ThierryA

ThierryA Dec 13, 2017

Collaborator

I can't think of a case where this would be needed as a fallback rather than directly getting the images like we do for the other shortcodes (please advise if I am missing something). This commit simplifies this.

@ThierryA

This comment has been minimized.

Collaborator

ThierryA commented Dec 13, 2017

@kienstra @westonruter I think it would be great to document this script and steps to run it somewhere, whether it is in the contributing.md file and/or open an other issue (ex. "Support vanilla embeds") for the actual work AMP support work and refer to it in this PR description. Thoughs?

Fix rendering of SoundCloud embeds and correct parameters for soundcl…
…oud shortcode

* Hook into oEmbed filters for rewriting SoundCloud embeds as AMP components.
* Ensure Jetpack is active while unit tests are run.
* Explicitly install and activate Jetpack during unit tests.
* Fix error message in create-embed-test-post.php
* Fix phpcs for Soundcloud code.

@amedina amedina changed the title from Issue 806 : Add skeleton for create-embed-test-post script to Add skeleton for create-embed-test-post script Dec 14, 2017

@amedina

This comment has been minimized.

Collaborator

amedina commented on 0b729b1 Dec 14, 2017

Thanks for fixing the SoundCloud embed @westonruter !

westonruter added some commits Dec 14, 2017

Fix handling of polldaddy oEmbeds in AMP
* Use noscript response from oEmbed as default content.
* Include WPCOM_AMP_Polldaddy_Embed by default even when not on WordPress.com; deprecate wpcom/shortcodes.php.
* Fix DailyMotion and FunnyOrDie test URLs.
* Clean phpcs for class-amp-polldaddy-embed.php

@westonruter westonruter changed the title from Add skeleton for create-embed-test-post script to Add create-embed-test-post script and fix support for various embeds Dec 14, 2017

*/
public function filter_embed_oembed_html( $cache, $url, $attr ) {
$parsed_url = wp_parse_url( $url );
if ( $url && false === strpos( $parsed_url['host'], 'polldaddy.com' ) ) {

This comment has been minimized.

@westonruter

westonruter Dec 14, 2017

Collaborator

This may need to include looking at poll.fm, but I've never seen any PollDaddy URLs look like that: https://github.com/WordPress/wordpress-develop/blob/c6326694380722ccbdd78e21a2bd0d079383291c/src/wp-includes/class-oembed.php#L69

This comment has been minimized.

@ThierryA

ThierryA Dec 14, 2017

Collaborator

While the risk is minimal, this could throw a PHP warning if the url passed doesn't have an host.

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Dec 14, 2017

Documenting wp-cli Script In contributing.md

Hi @ThierryA,

I think it would be great to document this script and steps to run it somewhere, whether it is in the contributing.md file and/or open an other issue...

That's a good idea. If it's alright, I'll add steps to use the wp-cli script in contributing.md.

Also, I need to add the oEmbed URLs that @westonruter requested.

'Please make sure at least %1$s "%2$s" attachments are accessible and run this script again.',
$image_count,
$type,
$posts_count

This comment has been minimized.

@ThierryA

ThierryA Dec 14, 2017

Collaborator

This can be removed now that the message changed.

This comment has been minimized.

@westonruter

westonruter Dec 14, 2017

Collaborator

Oh, I will restore it. I also realized we should use $query->found_posts and $query->post_count here.

@@ -9,22 +9,49 @@
* Class AMP_SoundCloud_Embed_Handler
*/
class AMP_SoundCloud_Embed_Handler extends AMP_Base_Embed_Handler {

This comment has been minimized.

@ThierryA

ThierryA Dec 14, 2017

Collaborator

Adding the @since tag for new code and changes would be helpful.

private function render_embed_fallback( $url ) {
return AMP_HTML_Utils::build_tag( 'a',
array(
'href' => esc_url( $url ),

This comment has been minimized.

@ThierryA

ThierryA Dec 14, 2017

Collaborator

I am not 100% sure why we excluded this rule. It is now inconsistent throughout the project which doesn't really make sense since we require the variable operators to be aligned.

PS: this is indeed not a blocker but I think it would be better to keep the code style as consistent as possible.

This comment has been minimized.

@westonruter

westonruter Dec 14, 2017

Collaborator

Yeah, I agree. I'll remove the WordPress.Arrays.MultipleStatementAlignment.DoubleArrowNotAligned suppression.

* So on WP<4.9 we set a post global to ensure oEmbeds get processed.
*/
if ( version_compare( strtok( get_bloginfo( 'version' ), '-' ), '4.9', '<' ) ) {
$post = get_post( $this->factory()->post->create() );

This comment has been minimized.

@ThierryA

ThierryA Dec 14, 2017

Collaborator

Tips: get_post( $this->factory()->post->create() ) can be replaced with $this->factory()->post->create_and_get().

*/
public function filter_embed_oembed_html( $cache, $url ) {
$parsed_url = wp_parse_url( $url );
if ( $url && false === strpos( $parsed_url['host'], 'soundcloud.com' ) ) {

This comment has been minimized.

@ThierryA

ThierryA Dec 14, 2017

Collaborator

While the risk is minimal, this could throw a PHP warning if the url passed doesn't have an host.

This comment has been minimized.

@westonruter

westonruter Dec 14, 2017

Collaborator

I don't believe this can ever happen.

*/
public function filter_embed_oembed_html( $cache, $url, $attr ) {
$parsed_url = wp_parse_url( $url );
if ( $url && false === strpos( $parsed_url['host'], 'polldaddy.com' ) ) {

This comment has been minimized.

@ThierryA

ThierryA Dec 14, 2017

Collaborator

While the risk is minimal, this could throw a PHP warning if the url passed doesn't have an host.

@ThierryA

This comment has been minimized.

Collaborator

ThierryA commented Dec 14, 2017

That's a good idea. If it's alright, I'll make add steps to use the wp-cli script in contributing.md.

@kienstra sounds good to me.
@westonruter I added a round of CR, nothing major.

kienstra added some commits Dec 14, 2017

Issue 806 : Add instructions for using the wp-cli test page script.
Add steps to contributing.md to use create-embed-test-post.php.
Include a step that this should be inside an environment like VVV.
@westonruter

This comment has been minimized.

Collaborator

westonruter commented Dec 14, 2017

I noticed that the iframe for Funny Or Die is getting stripped out, leaving behind just:

I Still Haven't Found the Droids I'm Looking For – watch more funny videos

We need to figure out why the AMP_Iframe_Sanitizer isn't applying here as expected.

kienstra and others added some commits Dec 14, 2017

Issue 804 : Add Amazon, Animoto, and Speakerdeck URLs
Add these URLs to test oEmbed.
Use the Amazon testing URLs in:
https://core.trac.wordpress.org/ticket/38181
Props @jsepia for these URLs.
Remove DoubleArrowNotAligned PHPCS suppression
* Add missing since tag
* Update filter_embed_oembed_html calls since $url is required
* Add DEFAULT_BASE_BRANCH=develop
* Use create_and_get in test
Issue 806 : Update the Photobucket and Scribd URLs.
Thanks to ohthenoes for the Photobucket link in:
https://core.trac.wordpress.org/ticket/13754
And thanks to @wonderboymusic for the Scribd URL from:
https://core.trac.wordpress.org/ticket/28379.
Issue 806 : Merge in feature branch, resolve conflicts.
The only conflict was from array alignment.
It was a whitespace-only edit.
Issue 806 : Remove Vine embed, add Screencast embed.
Add a Screencast embed, thanks to designsimply's URL in:
https://core.trac.wordpress.org/ticket/24660
Because Vine isn't supported anymore, remove its test URL.
@see wordpress-develop/src/wp-includes/class-oembed.php
Issue 806 : Add tests for Someecards.
Both the full URL, and the short URL ending with the TLD .ly.
They both render well in AMP.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Dec 14, 2017

Tests For All oEmbed Providers

Hi @westonruter,
Thanks for your patience. There are now tests for all WordPress oEmbed providers in create-embed-test-post.php.

Still, some of them aren't rendering properly, even on a non-AMP page. Including Imgur, Photobucket, and Screencast. I'm working on those now.

kienstra added some commits Dec 14, 2017

Issue 806 : Correct embeds, including Imgur, Polldaddy, Screencast.
Change the URLs for most of these.
And remove the soundcloud shortcode.
This isn't supported natively.
Issue 806 : Update Photobucket, Tumblr, remove Imgur.
Use mrdenn1s's image from his Trac ticket:
https://core.trac.wordpress.org/ticket/28345
It looks like Imgur oEmbeds no longer work in WordPress.
@see https://core.trac.wordpress.org/ticket/42247
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Dec 14, 2017

Tests For Almost All oEmbeds

Hi @westonruter,
This now has tests for almost all oEmbeds. Except for Imgur, which seems to not be supported.

@westonruter westonruter added this to the v0.6 milestone Dec 14, 2017

@westonruter westonruter merged commit 78c60fa into develop Dec 14, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/806-add-script-for-testing-content branch Feb 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment