search

Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.59k stars 799 forks source link

Blocks: audit codebase for any use of the Automattic\Jetpack\Extensions\ namespace when blocks are disabled #38746

Closed jeherve closed 2 months ago

jeherve commented 2 months ago

Folks can disable Jetpack Blocks in Jetpack > Settings > Writing. When that happens, we do not include any of the blocks files:

https://github.com/Automattic/jetpack/blob/947adfb39deadc9398d116c0d8ef5623226cc005/projects/plugins/jetpack/class.jetpack-gutenberg.php#L797-L814

Thus, it can cause issues when in other parts of the codebase, we rely on functions and classes that should have been made available but weren't because blocks are disabled. #38742 is a good example of that.

It would be worth auditing our codebase and ensuring we're not causing any more such Fatal errors.

jeherve commented 2 months ago

@Automattic/zap Tagging y'all on this one since I believe this could impact some of the Subscription features?

simison commented 2 months ago

Yep, this would be big problem for subscriptions.

Sounds like what's needed are multiple block bundles as per Jetstream boundaries.

darssen commented 2 months ago

Did some auditing, and found that https://github.com/Automattic/jetpack/pull/34602 fixed one scenario for Subscriptions. Also found these const usages, but they do have the require just below.

The only scenario I found that I think needs fixing I took care in this PR

@jeherve I basically did a Search of \Extensions in the monorepo and made sure any usage outside of blocks had the pertinent requires, is this sufficient, or am I missing something obvious?

jeherve commented 2 months ago

I basically did a Search of \Extensions in the monorepo and made sure any usage outside of blocks had the pertinent requires, is this sufficient, or am I missing something obvious?

That would have been my approach as well. If you didn't find any other uses of it, then this could be closed I think!

darssen commented 2 months ago

Great, then I'll leave it to the aforementioned PR to close this one once merged. Thanks!