Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#45447 closed defect (bug) (fixed)

Add Embed Preview support for classic embed providers

Reported by: swissspidy's profile swissspidy Owned by: swissspidy's profile swissspidy
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.0
Component: Embeds Keywords: has-patch has-unit-tests
Focuses: rest-api Cc:

Description

In #45142 we ported over the logic from gutenberg_filter_oembed_result() directly to core in \WP_oEmbed_Controller::get_proxy_item().

However, we somehow missed to add the changes from https://github.com/WordPress/gutenberg/pull/6345 as well. Basically, we didn't use the most up to date version of gutenberg_filter_oembed_result().

We also need to call \WP_Embed::shortcode() in \WP_oEmbed_Controller::get_proxy_item() to add back support for classic embeds.

Attachments (3)

45447.diff (1.2 KB) - added by swissspidy 5 years ago.
45447.2.diff (3.0 KB) - added by swissspidy 5 years ago.
45447.3.diff (6.9 KB) - added by swissspidy 4 years ago.

Download all attachments as: .zip

Change History (32)

@swissspidy
5 years ago

#1 @swissspidy
5 years ago

  • Keywords needs-unit-tests added

45447.diff is an untested copy of the current gutenberg_filter_oembed_result() function.

#2 @pento
5 years ago

  • Milestone changed from 5.0.1 to 5.0.2

#3 @swissspidy
5 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#4 @pento
5 years ago

  • Keywords needs-unit-tests needs-testing removed
  • Owner set to pento
  • Status changed from new to assigned

#5 @pento
5 years ago

  • Keywords needs-unit-tests needs-testing added
  • Milestone changed from 5.0.2 to 5.0.3
  • Owner pento deleted

Accidentally changed the keywords on this. :)

#6 @swissspidy
5 years ago

#45757 was marked as a duplicate.

#7 @audrasjb
5 years ago

  • Milestone changed from 5.0.3 to 5.1

5.0.3 is going to be released in a couple of weeks. We are currently sorting the remaining tickets in the milestone. It doesn't appear that ticket can be handled in the next couple of weeks (still needs testing and unit-tests). Let's address it in 5.1 which is coming in February. Feel free to change/ask to change the milestone if you think the issue can be quickly resolved.

@swissspidy
5 years ago

#8 @swissspidy
5 years ago

  • Keywords has-unit-tests dev-feedback added; needs-unit-tests needs-testing removed

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


5 years ago

#10 @swissspidy
5 years ago

  • Milestone changed from 5.1 to 5.2

With 5.1 Beta 1 around the corner I guess we have to punt this.

This ticket was mentioned in Slack in #core-restapi by desrosj. View the logs.


5 years ago

#12 @kadamwhite
5 years ago

  • Owner set to kadamwhite
  • Status changed from assigned to accepted

#13 @swissspidy
5 years ago

#46138 was marked as a duplicate.

#14 @aduth
5 years ago

Related:

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-restapi by kadamwhite. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


5 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


5 years ago

#19 @JeffPaul
5 years ago

  • Milestone changed from 5.2 to Future Release

Per input from @swissspidy and @kadamwhite in today's bugscrub, we're punting this as not enough time remains to get this into 5.2. Given the minimal movement during the 5.2 release cycle, I'm punting this to Future Release so that we consciously milestone this for a numbered release when we're ready to take it on.

#20 @jrchamp
4 years ago

Hi all - read through this ticket and the linked slack logs. @swissspidy has had some version of a patch available for a year and then regrettably, it's been punt, punt, punt until now. Based on the lack of references, I'm concerned it's not even on the radar.

@kadamwhite It sounds like you've done work on this, is it possible to share a status publicly so people can help or comment on the direction/progress?

Our users are actively complaining when they see this error message because they think we broke something.

#21 @thrijith
4 years ago

Trying to add a custom embed that is being handled by wp_embed_register_handler isn't working in the embed block, tested the patch by @swissspidy in 5.3.2 and it fixes the issue. If possible, I would like to know if Is there a plan to include this in any near release?

#22 @swissspidy
4 years ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Future Release to 5.5

A bit fuzzy on the details here about why this never went anywhere.

Since this patch still applies cleanly and tests pass, no reason why we couldn't include it.

#23 @TimothyBlynJacobs
4 years ago

This looks good to me from a REST API perspective.

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


4 years ago

#25 @whyisjake
4 years ago

  • Keywords commit removed

I'm getting the following errors when running tests.

1) Test_oEmbed_Controller::test_proxy_with_invalid_oembed_provider_no_discovery
Failed asserting that 1 matches expected 0.

/tests/phpunit/tests/oembed/controller.php:646

2) Test_oEmbed_Controller::test_proxy_with_invalid_oembed_provider_with_default_discover_param
Failed asserting that 2 matches expected 1.

/tests/phpunit/tests/oembed/controller.php:657

#26 @swissspidy
4 years ago

  • Owner changed from kadamwhite to swissspidy
  • Status changed from accepted to assigned

#27 @swissspidy
4 years ago

  • Status changed from assigned to accepted

@swissspidy
4 years ago

#28 @swissspidy
4 years ago

@whyisjake added an updated patch. happy to commit if you approve.

#29 @whyisjake
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 48135:

Embeds: Ensure that classic embed support works in the block editor.

See https://github.com/WordPress/gutenberg/pull/6345

Fixes #45447.

Props swisspidy, pento, audrasjb, aduth, jrchamp, thrijith, TimothyBlynJacobs, whyisjake.

Note: See TracTickets for help on using tickets.