Skip to content
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

Blocks: Add Facebook and Instagram Embed block variations #17192

Merged
merged 18 commits into from
Sep 30, 2020

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Sep 17, 2020

"Successor" to D49605-code and 522-gh-wpcomsh. Once this is merged to JP, and the corresponding WP.com diff (D49756-code) is committed to WP.com, we can retire those changes.

Changes proposed in this Pull Request:

Quoting 522-gh-wpcomsh:

Register a callback for the block.registerBlockType frontend filter that patches the Facebook and Instagram core/embed block variations so they appear in the inserter and can have their relevant URLs parsed again.

This is relevant for Gutenberg >= 9.0 (see CfT), where the aforementioned blocks were deprecated because they do not support the oEmbed changes that Facebook is going to implement on Oct. 24th, 2020, and core has currently no plans to support it.

However, we do plan on keeping support for them on WPCOM/AT. Facebook is adding a token authentication mechanism to its new oEmbed implementation, and we are following suit.

Our goal is for this go unnoticed by our end-users, as this is only an implementation detail.

Jetpack product discussion

p7DVsv-9jU-p2#comment-31116

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Make sure you're testing on a WP install with Gutenberg 9.0 installed.

This diff is rather simple -- the crucial part is to make sure that we're actually not making any more requests to the legacy API endpoints for Instagram oEmbeds (https://api.instagram.com/oembed). We'll be testing that quite thoroughly in the following.

The Facebook embeds OTOH are largely unaffected, since they haven't been previewed in the editor before (instead, we've been displaying an Embedded content from facebook.com can't be previewed in the editor. message in the block). (Some discussion on whether that's the way it should be here: Automattic/wp-calypso#27701 (comment), but that's not really relevant to this PR.)

  • We need to direct the REST API requests your local Jetpack dev install makes to your WP.com sandbox: In your local Jetpack dev env, sandbox the REST API, e.g. by adding a 0-sandbox.php file to docker/mu-plugins/, with the following contents:
<?php

define( 'JETPACK__SANDBOX_DOMAIN', 'YOURSANDBOX' );

(change YOURSANDBOX to your sandbox' URL). Make sure to restart your JP install afterwards. For more information on this, see PCYsg-efV-p2 (Section "The JETPACK__SANDBOX_DOMAIN PHP Constant").

  • Make sure that your local JP install is really using your sandbox for these requests: Log out of your sandbox, and paste the link of an Instagram post into your JP install's Gutenberg editor. You should get an empty embed block showing a notice saying something like "Sorry, this content could not be embedded".
  • Log into your sandbox, and verify that pasting an IG post URL is now successfully transformed into an embed. Use a different IG post to avoid getting a cached result.
  • Apply the following patch to your sandbox to simulate the retiring of Instagram's legacy API endpoint:
Index: wp-content/rest-api-plugins/endpoints/oembed-proxy.php
===================================================================
--- wp-content/rest-api-plugins/endpoints/oembed-proxy.php	(revision 213803)
+++ wp-content/rest-api-plugins/endpoints/oembed-proxy.php	(working copy)
@@ -52,8 +52,10 @@
 				'https://graph.facebook.com/v5.0/instagram_oembed'
 			);
 		} else {
+			$query_args = $request->get_query_params();
+			$query_args['breaking_change'] = 'oembed';
 			$request_url = add_query_arg(
-				$request->get_query_params(),
+				$query_args,
 				'https://api.instagram.com/oembed/'
 			);
 		}
  • Try inserting another IG post URL (again a different one, because caching). It should fail.
  • Fortunately, we already have code in place to use the new oEmbed API instead. To enable it, set
$auth_to_instagram = true;

on L40 of wp-content/rest-api-plugins/endpoints/oembed-proxy.php in your sandbox.
(I can't post a diff here, since it would include an internal link.)

  • Verify that the block embed works now.
  • Finally, we need to simulate WP.com behavior after the legacy oEmbed API has been retired. To that end, apply the following patch to your local JP install (and reload the block editor):
diff --git a/modules/shortcodes/instagram.php b/modules/shortcodes/instagram.php
index 8a24c479b..bb80ed709 100644
--- a/modules/shortcodes/instagram.php
+++ b/modules/shortcodes/instagram.php
@@ -248,7 +248,8 @@ function jetpack_instagram_use_cache( $matches, $atts, $passed_url ) {
  * @return mixed An object if successful or a WP_Error object
  */
 function jetpack_instagram_fetch_embed( $args ) {
-       if ( Constants::is_defined( 'IS_WPCOM' ) && Constants::get_constant( 'IS_WPCOM' ) ) {
+       if ( true ) {
+               $args['breaking_change'] = 'oembed';
                $url      = esc_url_raw(
                        add_query_arg(
                                $args,
  • Verify that pasting an IG URL doesn't work now. This means that we'll have to modify the if clause to make sure we're also using the new API for WP.com after Oct 24.
  • Remove all patches that you've applied on top of this branch, or on your sandbox. Disconnect your JP from WP.com (at /wp-admin/admin.php?page=jetpack#/dashboard, "Manage website connection"). Turn on Offline Mode. Reload the editor, and verify that the IG embed is gone from the inserter, while the FB one is still there.

Conclusion from Testing Instructions

  • The blocks added by this PR currently work -- the PR can be merged as-is ✔️
  • To prepare for Facebook's retiring of the old oEmbed API, we need to set $auth_to_instagram = true in wp-content/rest-api-plugins/endpoints/oembed-proxy.php before Oct 24. (We know this much is true.)
  • To also make sure that the block variations work on WP.com after Oct 24, we need to file a PR that modifies the conditional in jetpack_instagram_fetch_embed in modules/shortcodes/instagram.php accordingly.

Proposed changelog entry for your changes:

Add Facebook and Instagram Embed block variations.

cc/ @Automattic/good-mountain @ebinnion @jeherve

Fixes Automattic/wp-calypso#45832

@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello ockham! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D49756-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@jetpackbot
Copy link

jetpackbot commented Sep 17, 2020

Warnings
⚠️

pre-commit hook was skipped for one or more commits

E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-17192

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against b69ecbc

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worth moving this to extensions/extended-blocks, since we're extending a core block (vs. creating our own block).

See #17137 for an example.

@ockham
Copy link
Contributor Author

ockham commented Sep 17, 2020

It may be worth moving this to extensions/extended-blocks, since we're extending a core block (vs. creating our own block).

See #17137 for an example.

Ah thanks, good point!

@ockham
Copy link
Contributor Author

ockham commented Sep 18, 2020

It may be worth moving this to extensions/extended-blocks, since we're extending a core block (vs. creating our own block).

f6216e6

@ockham ockham marked this pull request as ready for review September 18, 2020 12:26
@ockham ockham requested a review from a team September 18, 2020 12:26
@ockham ockham added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Sep 18, 2020
@jeherve jeherve added this to the 9.0 milestone Sep 18, 2020
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 18, 2020
@ockham ockham added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 18, 2020
@jeherve
Copy link
Member

jeherve commented Sep 23, 2020

I may be missing something here, but since the embed registration will also be removed from Core in 50861-core, won't we be creating issues on sites where Jetpack's Shortcodes module (where both Facebook and Instagram embeds are registered) is deactivated? On those sites, we'll show the 2 embed variations, but won't have the embed registration happening behind the scenes.

This may happen on a lot of sites since the module is not activated by default.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 23, 2020
}

addFilter(
'blocks.registerBlockType',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we use registerBlockVariation() as default and use the filter method as a fallback?

@ockham
Copy link
Contributor Author

ockham commented Sep 28, 2020

I may be missing something here, but since the embed registration will also be removed from Core in 50861-core, won't we be creating issues on sites where Jetpack's Shortcodes module (where both Facebook and Instagram embeds are registered) is deactivated? On those sites, we'll show the 2 embed variations, but won't have the embed registration happening behind the scenes.

This may happen on a lot of sites since the module is not activated by default.

That's a great point, thanks!

Are we exposing the activation status of JP modules through a REST API endpoint already? (Can't remember off the top of my head 🤔 ) Otherwise I'll need to expose this some other way...

@ockham ockham force-pushed the add/facebook-and-instagram-embed-variations branch from f6216e6 to 815a23d Compare September 28, 2020 09:55
@jeherve
Copy link
Member

jeherve commented Sep 28, 2020

Are we exposing the activation status of JP modules through a REST API endpoint already? (Can't remember off the top of my head 🤔 ) Otherwise I'll need to expose this some other way...

There are a few endpoints you could look at, and it may be worth creating a new shared utility in extensions/shared to fetch a module and its status:

// Return all modules
register_rest_route( 'jetpack/v4', '/module/all', array(
'methods' => WP_REST_Server::READABLE,
'callback' => array( $module_list_endpoint, 'process' ),
'permission_callback' => array( $module_list_endpoint, 'can_request' ),
) );
// Activate many modules
register_rest_route( 'jetpack/v4', '/module/all/active', array(
'methods' => WP_REST_Server::EDITABLE,
'callback' => array( $module_list_endpoint, 'process' ),
'permission_callback' => array( $module_list_endpoint, 'can_request' ),
'args' => array(
'modules' => array(
'default' => '',
'type' => 'array',
'items' => array(
'type' => 'string',
),
'required' => true,
'validate_callback' => __CLASS__ . '::validate_module_list',
),
'active' => array(
'default' => true,
'type' => 'boolean',
'required' => false,
'validate_callback' => __CLASS__ . '::validate_boolean',
),
)
) );
// Return a single module and update it when needed
register_rest_route( 'jetpack/v4', '/module/(?P<slug>[a-z\-]+)', array(
'methods' => WP_REST_Server::READABLE,
'callback' => array( $core_api_endpoint, 'process' ),
'permission_callback' => array( $core_api_endpoint, 'can_request' ),
) );

Or maybe we could pass the list of modules and their status here:

'available_blocks' => self::get_availability(),

@ockham ockham force-pushed the add/facebook-and-instagram-embed-variations branch from 9e4aea4 to 967c596 Compare September 30, 2020 12:12
@ockham
Copy link
Contributor Author

ockham commented Sep 30, 2020

Rebased.

@ockham
Copy link
Contributor Author

ockham commented Sep 30, 2020

About f7fcb81: we do in fact need that check, as blocks and those embed variations can be available on sites that are not connected to WordPress.com yet, but that use Offline mode. In those cases, we don't want the Instagram variation to be available.

Okay, I'll revert!

Done in b69ecbc.

@ockham ockham added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 30, 2020
@ockham ockham requested a review from jeherve September 30, 2020 12:38
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests well for me. Merging.

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 30, 2020
@jeherve jeherve merged commit 1dba135 into master Sep 30, 2020
@jeherve jeherve deleted the add/facebook-and-instagram-embed-variations branch September 30, 2020 16:12
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 30, 2020
jeherve added a commit that referenced this pull request Sep 30, 2020
@jeherve
Copy link
Member

jeherve commented Sep 30, 2020

Cherry-picked to branch-9.0 in f1cd070

I'll let you deal with the WordPress.com counterpart?

@ockham
Copy link
Contributor Author

ockham commented Sep 30, 2020

Cherry-picked to branch-9.0 in f1cd070

Thanks @jeherve !

I'll let you deal with the WordPress.com counterpart?

Will do! It's kinda blocked by D49389-code, but I'll work around that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Shortcodes / Embeds Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gutenberg: Ensure Facebook and Instagram embeds persist if removed from core
7 participants