Jetpack_Gutenberg: Decouple extensions registration from availability

[ockham]

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]

D23553-code. (newly created revision)

[jetpackbot]

	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]

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]

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.

[dmsnell]

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.

[ockham]

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.

[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).

[ockham]

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]

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

[ockham]

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]

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

[matticbot]

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

[matticbot]

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

[ockham]

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.

[jeherve]

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

[matticbot]

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