Jetpack_Gutenberg: Decouple extensions registration from availability by ockham · Pull Request #11212 · Automattic/jetpack

ockham · 2019-01-26T17:52:23Z

Changes proposed in this Pull Request:

jetpack_register_block() has been a pretty thin wrapper around (Gutenberg's) register_block_type(), and would alternatively call jetpack_set_extension_unavailability_reason() (with reason not_whitelisted) if a block wasn't whitelisted. However, we were already calling jetpack_set_extension_unavailability_reason() manually to specify different reasons.

This PR decouples extension registration and availability setting even further, to allow us to drop jetpack_register_block() altogether and directly use register_block_type() (with jetpack/-prefixed block names!) instead.

It also replaces register_jetpack_plugin() with the more generic jetpack_set_extension_available(), and jetpack_set_extension_unavailability_reason with jetpack_set_extension_available(). Note that both methods now require explicit use of the jetpack- prefix (or jetpack- for blocks).

It's still not quite as symmetric as I would like it to have ideally, since block availability is determined by block registration -- i.e. use jetpack_set_extension_available only for non-blocks, and register_block_type for blocks. However, use jetpack_set_extension_unavailable for both.

This is mostly because Gutenberg has the concept of a server-side block registration, but not for plugins. However, I think this should be a good enough compromise, since it should cater for the VideoPress situation, and it's untangling some concepts a bit better.

I think this also lays the groundwork for making jetpack/ and jetpack- prefixes explicit everywhere.

The chief motivation for this PR is to provide VideoPress with the ability to set its availability, since it is going to be a bit of a special case (drop-in replacement for core/video block), so our previous abstraction didn't quite hold.

Testing instructions:

Same as #11091

Proposed changelog entry for your changes:

TBD

/cc @mmtr

matticbot · 2019-01-26T17:52:27Z

D23553-code. (newly created revision)

jetpackbot · 2019-01-26T17:59:15Z

	Warnings
⚠️	The PR is missing at least one `[Status]` label. Suggestions: `[Status] In Progress`, `[Status] Needs Review`

This is 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 ae23942

ockham · 2019-01-28T10:58:21Z

Seeing a bunch of unit test failures here, mostly from Contact Form (yarn docker:phpunit --filter=WP_Test_Grunion_Contact_Form). We might need to unregister that block (and its child blocks) in WP_Test_Grunion_Contact_Form's tearDown (or tearDownAfterClass).

ockham · 2019-01-28T11:19:58Z

So this

diff --git a/tests/php/modules/contact-form/test_class.grunion-contact-form.php b/tests/php/modules/contact-form/test_class.grunion-contact-form.php
index 594550791..264069e93 100644
--- a/tests/php/modules/contact-form/test_class.grunion-contact-form.php
+++ b/tests/php/modules/contact-form/test_class.grunion-contact-form.php
@@ -58,6 +58,15 @@ class WP_Test_Grunion_Contact_Form extends WP_UnitTestCase {
                remove_all_filters( 'wp_mail' );
                remove_all_filters( 'grunion_still_email_spam' );
                remove_all_filters( 'jetpack_contact_form_is_spam' );
+
+               if ( class_exists( 'WP_Block_Type_Registry' ) ) {
+                       $blocks = WP_Block_Type_Registry::get_instance()->get_all_registered();
+                       foreach ( $blocks as $block_name => $block ) {
+                               if ( $block_name === 'jetpack/contact-form' || ( isset( $block->parent ) && in_array( 'jetpack/contact-form', $block->parent ) ) ) {
+                                       unregister_block_type( $block_name );
+                               }
+                       }
+               }
        }
 
        private function add_field_values( $values ) {

gets us down from 42 failures to 6, but I'm still not quite sure about the remaining 6, so this might not be entirely spot-on.

class.jetpack-gutenberg.php

dmsnell · 2019-01-28T20:57:15Z

class.jetpack-gutenberg.php

can we add a filter here so that this warns developers in development mode?

private static function remove_extension_prefix( $extension_name ) { if ( ! wp_startswith( $extension_name, 'jetpack/' ) ) { return $extension_name; } do_action( 'jetpack_gutenberg_extension_missing_prefix', $extension_name ); return substr( $extension_name, strlen( 'jetpack/' ) ); }

maybe my logic is bad or I don't understand what's going on. my point is that maybe we can do something where in our local machines during development work we can hook into this action and either kill the script or flag a big warning somehow to help prevent us from propagating two ways of naming things.

I'm hoping to hard-wire the prefixes back in soon after landing this PR. TBH I don't know what kind of development mode tooling JP offers (does it?) so I'd rather forgo for now 😬 I promise to keep an eye on these.

class.jetpack-gutenberg.php

modules/blocks.php

mmtr

Thanks @ockham! This looks promising 🎉

In order to avoid issues, I think we shouldn't register blocks if they are not whitelisted (or at least unregister them if we detect they haven registered).

class.jetpack-gutenberg.php

ockham · 2019-01-29T10:24:53Z

In order to avoid issues, I think we shouldn't register blocks if they are not whitelisted (or at least unregister them if we detect they haven registered).

how can a block be registered without being whitelisted? shouldn't we prevent to register (or unregister) it if the extension is not whitelisted?

Yeah, this is kind of the crux of this PR (and why I had it different before).

Here's the rationale:

We want the endpoint to expose unwhitelisted blocks (as unavailable). The reason is the fallback on the Calypso side that I added at some point that marks a beta block as available if it's not listed by the endpoint at all -- to enable developers to more quickly start writing a block (without requiring a Jetpack or WP.com patch). Conversely, that means that once a Calypso PR like that is merged (and thus the index.json which at that point contains the new block in the beta section), we use that information to hide the beta block -- by having the endpoint list positively list it, but as unavailable. (Should we revisit this once Gutenlypso is no more? Possibly, but let's do these things one at a time.)

On the other hand, since this PR is all about decoupling registration from availability setting, we can no longer intercept registration if a block/plugin isn't whitelisted. This was holding me back for a long time from making this change. However, I've come to the conclusion that this is probably not much of a problem:

AFAICT, block registration in Gutenberg isn't really a costly operation -- it pretty much just adds block names and settings to an array (not much different from what our own Jetpack_Gutenberg class does). Furthermore, it only has implications for the frontend (correct rendering of blocks), but the worst that can happen there is that blocks are displayed on the frontend even though they're unavailable -- which isn't the end of the world: Blocks will only be present in posts if a user has been able to insert them into that post through the editor, which is only possible if it's set as available by the endpoint. (There might be edge cases where a user inserted a block that's only available as part of a premium plan which they then disabled and the block is still available in the frontend, but TBH this is fine as a fallback, until we sort out fringe cases like these, as they might need UI design input.)

jeherve · 2019-02-01T14:21:55Z

The Contact Form tests are failing right now:
https://travis-ci.org/Automattic/jetpack/jobs/486584079

ockham · 2019-02-01T15:47:43Z

The Contact Form tests are failing right now:
https://travis-ci.org/Automattic/jetpack/jobs/486584079

Yeah, I know 🙁 Sry, should've reset the PR to 'In Progress' myself.

matticbot · 2019-02-08T12:04:21Z

ockham, Your synced wpcom patch D23553-code has been updated.

matticbot · 2019-02-08T12:09:02Z

ockham, Your synced wpcom patch D23553-code has been updated.

matticbot · 2019-02-08T12:17:22Z

ockham, Your synced wpcom patch D23553-code has been updated.

ockham · 2019-02-08T12:34:02Z

I think I've addressed all feedback -- this should be ready for another look.

mmtr

Manually tested all the extensions with different availability combinations in both environments Jetpack and WP.com and everything worked as expected. Thanks @ockham for this refactor!

Left only a non-blocking comment regarding code format.

class.jetpack-gutenberg.php

jeherve

This looks good and tests well on my end. 🚢!

Co-Authored-By: ockham <ockham@raz.or.at>

matticbot · 2019-02-08T15:30:31Z

ockham, Your synced wpcom patch D23553-code has been updated.

ockham added [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Jan 26, 2019

ockham self-assigned this Jan 26, 2019

ockham requested a review from a team January 26, 2019 17:52

matticbot added the Touches WP.com Files label Jan 26, 2019

ockham force-pushed the refactor/gutenberg-extension-availability branch from b3f5be4 to d051aa6 Compare January 26, 2019 17:55

ockham force-pushed the refactor/gutenberg-extension-availability branch from d051aa6 to 281c39b Compare January 26, 2019 18:13

ockham mentioned this pull request Jan 26, 2019

Jetpack_Gutenberg: Decrease abstraction when registering extensions #11176

Closed

ockham added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Jan 27, 2019

ockham requested a review from a team January 27, 2019 19:03

dmsnell reviewed Jan 28, 2019

View reviewed changes

class.jetpack-gutenberg.php Outdated Show resolved Hide resolved

dmsnell reviewed Jan 28, 2019

View reviewed changes

class.jetpack-gutenberg.php Outdated Show resolved Hide resolved

dmsnell reviewed Jan 28, 2019

View reviewed changes

class.jetpack-gutenberg.php Outdated Show resolved Hide resolved

dmsnell reviewed Jan 28, 2019

View reviewed changes

modules/blocks.php Outdated Show resolved Hide resolved

mmtr reviewed Jan 29, 2019

View reviewed changes

class.jetpack-gutenberg.php Outdated Show resolved Hide resolved

ockham force-pushed the refactor/gutenberg-extension-availability branch 3 times, most recently from d8a90d4 to e79379a Compare January 30, 2019 19:37

jeherve added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Feb 1, 2019

ockham force-pushed the refactor/gutenberg-extension-availability branch from e79379a to e2b80c3 Compare February 4, 2019 13:35

Add two more register_block_type() checks

3f93ab5

Missing closing paren in a comment

d188e9d

Fix remove_extension_prefix

b3d9ff4

mmtr previously approved these changes Feb 8, 2019

View reviewed changes

class.jetpack-gutenberg.php Outdated Show resolved Hide resolved

simison added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 8, 2019

jeherve previously approved these changes Feb 8, 2019

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Feb 8, 2019

Whitespace fix

ae23942

Co-Authored-By: ockham <ockham@raz.or.at>

ockham dismissed stale reviews from jeherve and mmtr via ae23942 February 8, 2019 15:30

ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 8, 2019

eliorivero approved these changes Feb 8, 2019

View reviewed changes

ockham merged commit 2d3538f into master Feb 8, 2019

matticbot added the [Status] Needs Changelog label Feb 8, 2019

ockham deleted the refactor/gutenberg-extension-availability branch February 8, 2019 15:49

jeherve mentioned this pull request Feb 8, 2019

Gutenberg: only load extra blocks if Gutenberg is available. #11308

Merged

ockham mentioned this pull request Feb 8, 2019

Gutenberg: Add and use jetpack_register_block_type() wrapper #11310

Merged

simison mentioned this pull request Feb 11, 2019

Repeat Visitor: register new block and assets #11224

Merged

ockham mentioned this pull request Feb 11, 2019

Set the availability of the VideoPress extension for Gutenberg #11321

Merged

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog labels Feb 19, 2019

jeherve added a commit that referenced this pull request Feb 22, 2019

changelog: add #11212

46cc412

Conversation

ockham commented Jan 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

matticbot commented Jan 26, 2019

Uh oh!

jetpackbot commented Jan 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ockham commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ockham commented Jan 28, 2019

Uh oh!

Uh oh!

dmsnell Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

ockham Feb 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mmtr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ockham commented Jan 29, 2019

Uh oh!

jeherve commented Feb 1, 2019

Uh oh!

ockham commented Feb 1, 2019

Uh oh!

matticbot commented Feb 8, 2019

Uh oh!

matticbot commented Feb 8, 2019

Uh oh!

matticbot commented Feb 8, 2019

Uh oh!

ockham commented Feb 8, 2019

Uh oh!

mmtr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

matticbot commented Feb 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ockham commented Jan 26, 2019 •

edited

Loading

jetpackbot commented Jan 26, 2019 •

edited

Loading

ockham commented Jan 28, 2019 •

edited

Loading