Automatically enable FSE for FSE themes #26500
Conversation
youknowriad
requested review from
adamziel,
ajitbohra,
draganescu,
ellatrix,
kevin940726,
nerrad,
noisysocks,
ntwb,
Soean,
spacedmonkey,
talldan,
tellthemachines and
TimothyBJacobs
as code owners
youknowriad
requested review from
mtias,
gziolo,
noahtallen and
vindl
and removed request for
kevin940726 and
tellthemachines
packages/block-library/src/index.js
Outdated
| ? ( settings ) => { | ||
| const { __experimentalEnableFullSiteEditing } = settings; | ||
|
|
||
| ? ( isSiteEditor ) => { |
There was a problem hiding this comment.
This is not great and this is the main question/uncertainty I have for this PR: When should we enable the FSE blocks?
IMO, they should be enabled for both FSE and non-FSE themes and in both site editor and post editor. For example you could use a query block in the post editor if you want.
so maybe we can just hide these blocks behind the process.env.GUTENBERG_PHASE flag which means they are always enabled on Gutenberg and disabled on Core. The issue is that we didn't think much about their organization in the inserter yet.
There was a problem hiding this comment.
I'm generally fine registering the site blocks in all editors as they mature. See also #26316. I think several of the theme specific blocks are not quite ready to be fully exposed, though. Also wonder if we'd want a "Theme" category for all these new blocks.
There was a problem hiding this comment.
Wouldn't we have to ensure that you can't embed Post Content inside the edit post first? There might be other edge cases like can whether it's expected to embed Template Parts inside Post Content. Well, the sooner we enable the sooner we detect potential issues :)
There was a problem hiding this comment.
Template parts should be restricted to site editing, I don't see a reason to allow them elsewhere outside of causing extra confusion.
There was a problem hiding this comment.
One thought about this idea:
so maybe we can just hide these blocks behind the
process.env.GUTENBERG_PHASEflag which means they are always enabled on Gutenberg and disabled on Core. The issue is that we didn't think much about their organization in the inserter yet.
process.env.GUTENBERG_PHASE is compiled away. That means that sites using the package will not be able to opt-in or out of experimental blocks. If that's desirable it would need to remain as a setting.
There was a problem hiding this comment.
I think I found a better heuristic for the moment: enable these blocks in both post editor and site editor for FSE themes and disable them for regular themes.
The meaning of "FSE themes" might change and once these blocks become more stable, we'll have to revisit this and find better conditions.
|
Size Change: +207 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
I'm a bit afraid that this change might imply that FSE is ready. (Also in theory any theme could be used with FSE: just create a |
youknowriad
force-pushed
the
try/auto-enable-fse-for-fse-themes
branch
from
b56e205
to
8bd671f
Compare
|
@Copons Right now, when you enable FSE on non-FSE themes, you have a broken experience and when you don't enable it for FSE themes, you have a broken experience too. I think we should fix that (with this PR) To clarify the status of FSE themes, I added an admin notice visible when you use an FSE theme. |
| require dirname( __FILE__ ) . '/edit-site-page.php'; | ||
| require dirname( __FILE__ ) . '/edit-site-export.php'; | ||
| } | ||
| require dirname( __FILE__ ) . '/full-site-editing.php'; |
There was a problem hiding this comment.
Will loading these all the time create any issues for non-FSE themes? Should we just do a conditional gutenberg_is_experiment_enabled( 'gutenberg-full-site-editing' ) || gutenberg_is_fse_theme() for this?
It seems easier than all of the bail early checks within the loaded files.
There was a problem hiding this comment.
The locate_template function (gutenberg_is_fse_theme) triggers an error when called before init, that's why I removed the check.
There was a problem hiding this comment.
If I recall, I moved all of these to be conditionally loaded because if the WP_Template CPT exists, then certain template resolution code starts working automatically. This meant that anyone could create WP_Template CPT, and start using template resolution behavior on the front end regardless of whether the experiment was enabled, and regardless of the theme.
Basically, are we sure that the new theme check prevents that scenario?
There was a problem hiding this comment.
Yes, we're sure I tested but regardless, with the change here #26500 (comment) we can restore this.
There was a problem hiding this comment.
It turns out that even with the new implementation, it's too early to call the function. I think it doesn't matter anyway since all the hooks do early returns if it's not an FSE theme.
youknowriad
force-pushed
the
try/auto-enable-fse-for-fse-themes
branch
from
470a800
to
be1528e
Compare
| add_filter( 'pre_set_theme_mod_custom_logo', 'sync_site_logo_to_theme_mod' ); | ||
| add_filter( 'theme_mod_custom_logo', 'override_custom_logo_theme_mod' ); | ||
| } | ||
| register_block_type_from_metadata( |
There was a problem hiding this comment.
Note: This won't be exposed in the WordPress core until explicitly listed in here:
| @@ -97,7 +97,9 @@ export function initializeEditor( | |||
| ); | |||
| registerCoreBlocks(); | |||
| if ( process.env.GUTENBERG_PHASE === 2 ) { | |||
| __experimentalRegisterExperimentalCoreBlocks( settings ); | |||
| __experimentalRegisterExperimentalCoreBlocks( | |||
| settings.__unstableEnableFullSiteEditingBlocks | |||
There was a problem hiding this comment.
Good idea 👍
The only downside is that all those blocks are going to be always sent to the browser in the plugin :(
Still, it is exactly what we had before so it can be ignored.
|
It looks like the only FSE theme published to Theme Directory- Q depends on the https://themes.trac.wordpress.org/browser/q/0.5/includes/RequireGutenberg.php#L227 @aristath – any suggestion on how we could proceed to keep Q working? I see the following banner with this branch:
|
Fixed in aristath/q@c382ae5 |
lib/full-site-editing.php
Outdated
| * @return boolean Whether the current theme is an FSE theme or not. | ||
| */ | ||
| function gutenberg_is_fse_theme() { | ||
| return is_readable( locate_template( 'block-templates/index.html' ) ); |
There was a problem hiding this comment.
locate_template is doing some more stuff like calling load_template and requiring the found file. Do we really want all of that here, as opposed to just checking for the presence of the file?
There was a problem hiding this comment.
Good question, this is inspired by the work done for theme.json here https://github.com/WordPress/gutenberg/pull/20047/files#diff-0b8e98dead5542e95159bd3c482158111d3f16637e7eef9af13373358fa42dedR14 by @nosolosw
I didn't give it many thoughts. @nosolosw any particular reason for this there too?
There was a problem hiding this comment.
I think it's fine given that the template won't be loaded unless you do locate_template( template, true );, which is not the case here (see). However, if there's a prefered way to check for the file, that'd be good as well.
There was a problem hiding this comment.
I'm thinking that something as simple as
is_readable( STYLESHEETPATH . '/block-templates/index.html' );should work. I don't think we need theme-compat and TEMPLATEPATH fallbacks for new themes.
|
There still are some failing e2e tests related to FSE:
|
There was a problem hiding this comment.
Code changes look good and got enough eyes to proceed. E2e tests related to FSE pass now, the remaining failures are known issues that exist in the master branch. I'm approving this PR but I would be happy to hear thoughts from other folks working on the FSE projects for a long time.
| @@ -49,25 +49,20 @@ function gutenberg_reregister_core_block_types() { | |||
| ), | |||
| 'block_names' => array_merge( | |||
There was a problem hiding this comment.
Superfluous array_merge call :)
When you enable an FSE theme, it's already an opt-in behavior, so there's no real need for a dedicated experiment flag.
In this PR:
This doesn't mean FSE is not experimental anymore, it still is but there's no need for a flag for it.
Fixes #26503