Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#54910 new defect (bug)

5.9 Bug - Blank Home Page

Reported by: cantuaria's profile cantuaria Owned by:
Milestone: Awaiting Review Priority: normal
Severity: major Version: 5.9
Component: Themes Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

After upgrading to 5.9, a few of my sites started showing a blank screen on Home Page when the show_on_front option is set to Latest Posts. Trying the new Visual Editor would output "This block has encountered an error and cannot be previewed"

I discovered that this occurs if you have a index.html inside a templates folder inside your theme folder. I believe that the new UI editor is failling back to this file when the proper file is not found. When the file is removed, everything works as before and the Site Editor is disabled.

In my case, I always have an empty index.html file inside my themes and plugins file, to avoid directory browsing in webservers. It's not really empty, it have a HTML comment line, that's how I discovered that WordPress was loading the file.

Attachments (5)

54910.diff (1.2 KB) - added by costdev 3 years ago.
Check for block markup in templates/index.html or block-templates/index.html.
54910_1.diff (1.2 KB) - added by costdev 3 years ago.
Also cover non-block HTML in templates/index.html or block-templates/index.html.
54910_cache.diff (1.7 KB) - added by costdev 3 years ago.
Cache the result of WP_Theme->is_block_theme().
54910_wp_theme_cache.diff (2.2 KB) - added by costdev 3 years ago.
Use WP_Theme::cache_get()/add() and add the is_block_theme to the keys to delete in WP_Theme::cache_delete().
54910_str_contains.diff (2.1 KB) - added by costdev 3 years ago.
Use str_contains() instead of parse_blocks().

Download all attachments as: .zip

Change History (63)

#1 @sabernhardt
3 years ago

  • Component changed from General to Themes

Hi and thanks for the report!

The block-based themes require a readable index.html file in both one of the 'block-templates' and 'templates' directories, so you apparently had the two directories.

To avoid directory browsing, you could switch one or both of those index files to an index.php file.

Last edited 3 years ago by sabernhardt (previous) (diff)

#2 follow-up: @poena
3 years ago

I think we need a more robust way to identify if a theme *really* is a full site editing theme.
This problem was less prominent before the renaming of the block-templates folder to templates which is a more common folder name.

#3 in reply to: ↑ 2 @Ov3rfly
3 years ago

Replying to poena:

a more robust way to identify if a theme *really* is a full site editing theme.

Would suggest to use get_theme_support() to identify theme features.

#4 @cantuaria
3 years ago

I agree. My first approach to fix the issue was looking for a way to explicity tell that my theme does not support Full Site Editing. I couldn't find any, only then I started tracking to see which file from my theme was causing the conflict.

Using the default file (index.html) for this, doesn't seems really good in a backward compatibility sense, since "templates" is also a common folder name used across themes and plugins.

Maybe keeping only the "block-templates" folder or use the standard way to declare support add_theme_support() would be more concise.

#5 follow-up: @costdev
3 years ago

One option is just to require the full-site-editing tag, or similar, in style.css as an explicit way to denote a block theme.

Another way is a filter for WP_Theme->is_block_theme(). I submitted PR 2175 for #54829 which implements an is_block_theme filter.

If a legacy theme had templates/index.html along with templates/some_php_file.php, it could add the following:

<?php

add_filter( 'is_block_theme', '__return_false' );

Similarly, if a theme developer wanted to organise their folders differently, say site_templates/page.html, they could add the following filter to make sure WP_Theme->is_block_theme() still returned true:

<?php

add_filter( 'is_block_theme', '__return_true' );

Related train of thought on the second case above

Actually, I don't know if there's currently a way to specify an alternative templates folder. Is this available?

For example, what if someone wanted to organise their folders differently say, site_templates/page.html?

If this is already possible, does the theme still need to have templates/index.html for WP_Theme->is_block_theme() to return true?

If this isn't available, I don't believe there's any existing strict theme folder naming restrictions in Core and it would be beneficial to allow theme developers to organise their directories based on what suits them. That's a separate ticket in itself, but some food for thought when implementing a solution that allows theme developers to change the outcome of WP_Theme->is_block_theme() as needed.

Last edited 3 years ago by costdev (previous) (diff)

#6 in reply to: ↑ 5 @Ov3rfly
3 years ago

Replying to costdev:

If a legacy theme ..., it could add the following ...

Legacy themes should not have to add anything as they most likely never will be updated.

If new themes support new features like FSE or block-templates (as in #54917), they should use add_theme_support() - that's what this API was made for.

... it would be beneficial to allow theme developers to organise their directories based on what suits them ...

The existing API for theme features can handle things like that already, e.g. like this:

add_theme_support( 'full-site-editing', array(
    'templates_folder' => 'site_templates',
) );
Last edited 3 years ago by Ov3rfly (previous) (diff)

#7 @costdev
3 years ago

I agree that add_theme_support() feels like the most appropriate way forward.

#8 follow-up: @poena
3 years ago

I disagree with adding the theme support in the full site editing theme because the purpose of these type of themes is to be easier to create.

Long term, adding more requirements to the block theme would be against the end goal.

-Most theme support is already enabled for block themes.
-A functions.php file is not required for block themes, and adding this would make it required to include one.

Requiring a theme.json may be one way, but the same exception could still be triggered since classic themes can use theme.json.

The classic themes that have chosen to include html files are the exception, as these files are of a non standard format for classic themes.
The burden of this choice should not be placed on other WordPress theme authors.

Ideally WordPress should recognize the format automatically.
The second best is an opt-out in the classic theme not the block theme.

#9 in reply to: ↑ 8 @Ov3rfly
3 years ago

Replying to poena:

because the purpose of these type of themes is to be easier to create

It is not easier for existing users with existing sites, they were there first.

would be against the end goal.

The end goal is to provide continuously working websites when updates are installed, see also Our mission:

... The basic WordPress software is simple and predictable so you can easily get started. ...

Requirement of changes in legacy themes and plugins are not predictable.

A functions.php file is not required for block themes,

A functions.php file is also not required for legacy themes.

as these files are of a non standard format

There is no standard format for own files, there are no required folders in legacy themes.

The WordPress community has decided to use the existing WordPress installation base with millions and millions of legacy themes and plugins to roll out all the new theme features into.

There would have been the possibility to fork/create a completely new project BlockPress or similar and cutting all technical debt and backwards compatibility, some also have suggested this when Gutenberg was started, but the community has decided otherwise.

There also would have been the possibility to introduce a new wp-content/block-themes/ location for themes with breaking new features, but the community has decided otherwise.

Now everybody in the complete WordPress userbase has to live with this technical debt, both existing users and new creators.

If a new theme for whatever reasons can not use functions.php to call add_theme_support(), it would be simple as a core fallback to implement a new field in style.css header or theme.json which the core then interprets as add_theme_support( 'full-site-editing' ). Then the exiting API with get_theme_support() can be used everywhere (similar to #54917) to identify a full site editing theme.

An example how to add new fields in header can be seen in a recent change in WordPress 5.8, so the relevant code is already there and well tested.

Requirement of adding an opt-out in legacy themes is not an option.

#10 @cantuaria
3 years ago

What's the purpose of add_theme_support() if not used to check if a theme support a feature?

Even if WordPress never used .html files from the theme folder before, it's really irresponsible to assume that no theme will have it. It's a common file used in any web environment, so the correct assumption would be that it may contain, and it's far simpler using an existing API to verify the support than condemn all themes that use .html files.

This way of thinking may cause a lot of insecurity for developers - it's the same as say that any file, besides style.css & functions.php, in your theme, may break your site in a future WordPress update.

#11 @poena
3 years ago

My standpoint remains that we as theme authors are responsible for the files that we place in the theme.

WordPress expects the file to be used correctly and deliberatly.

Perhaps it could try to fallback more gracefully to the index.php in the themes root folder if there is no block markup in the index.html file.

#12 @costdev
3 years ago

Submitted a patch for consideration based on @poena's suggestion to check for block markup in index.html.

Edit
Updated the patch to add a check for wp_is_block_theme() before running locate_block_template() to prevent "Empty template: Index" from being output on the frontend when index.html is empty.

Note:

  • if ( $content && ! empty( parse_blocks( $content ) ) {} is overkill, as (bool) array() === false.
  • if ( $content && parse_blocks( $content ) ) {} should be enough.

I used the former to clearly indicate the intention, but this can be changed to the latter if preferred.

Last edited 3 years ago by costdev (previous) (diff)

@costdev
3 years ago

Check for block markup in templates/index.html or block-templates/index.html.

This ticket was mentioned in Slack in #core by costdev. View the logs.


3 years ago

This ticket was mentioned in Slack in #core-editor by costdev. View the logs.


3 years ago

#15 @audrasjb
3 years ago

  • Keywords has-patch added

The patch looks good to me. It doesn't add too much unnecessary checks since the condition is checked only if such file exists and is readable 👍

Last edited 3 years ago by audrasjb (previous) (diff)

#16 @peterwilsoncc
3 years ago

  • Keywords needs-unit-tests added

I like the approach here, are you able to add some unit tests for a theme with an empty index.html vs a block theme? It seems like something that could easily break in the future.

There's a number of themes included in the test suite in the tests/phpunit/data directory.

#17 @costdev
3 years ago

Yeah I can add unit tests 👍

I just noticed though that the patch doesn't exactly cover a populated index.html file that has non-block HTML in it, as parse_blocks() will parse non-block HTML as a block with blockName => NULL. Adding a modified patch with a way to achieve this - would appreciate another review of it.

However, I believe that shortcodes also have blockName => NULL, so this would fail if a template consisted of only shortcodes. There may be other instances where blockName => NULL for valid block templates, but I'm not aware of any.

@costdev
3 years ago

Also cover non-block HTML in templates/index.html or block-templates/index.html.

#18 @TimothyBlynJacobs
3 years ago

If we're going to be adding more work to the method than just checking if a file exists, I think it would probably be worthwhile to cache this data so the check only happens once.

@costdev
3 years ago

Cache the result of WP_Theme->is_block_theme().

#19 @TimothyBlynJacobs
3 years ago

Thanks @costdev! I think this should probably be using the WP_Theme::cache_add methods.

@costdev
3 years ago

Use WP_Theme::cache_get()/add() and add the is_block_theme to the keys to delete in WP_Theme::cache_delete().

#20 @costdev
3 years ago

Patch updated - thanks @TimothyBlynJacobs!

I'll pull this to a PR tomorrow and get started on unit tests.

#21 @TimothyBlynJacobs
3 years ago

Looks great to me, thanks @costdev!

#22 @costdev
3 years ago

I've created an issue upstream in the Gutenberg repo to get more insights from the editor team on block theme architectural concepts, performance, and to help prevent breaks now and in the future.

#23 @Mamaduka
3 years ago

@costdev

Maybe we should false !== strpos( $content, '<!-- wp:' ) instead of parsing the blocks. This method should be a little bit faster.

It's similar to what has_blocks uses under the hood - https://developer.wordpress.org/reference/functions/has_blocks/.

P.S. Thanks for adding conditional to the get_query_template that item was on my list.

Last edited 3 years ago by Mamaduka (previous) (diff)

#24 @swissspidy
3 years ago

I‘d also use has_blocks() here if possible

#25 @youknowriad
3 years ago

Hi there! thanks for catching that bug. I’d have loved to find this problem sooner now we can’t ask folks to change their themes because it would impact either block themes or classic themes.

On theme supports, the goal is make themes easy to created and also make them serializable (you can create without writing code) so ideally we want to remove the need for functions.php and php code entirely from block themes. Theme supports are slowly being replaced by theme.json equivalents in that sense.

Anyway, @mamaduka’s solution here seems like the simplest and most efficient one for me, and consistent with what we did before. Any downside to that?

#26 @andraganescu
3 years ago

One downside of checking templates/index.html for blocks via strpos or has_blocks is that a block theme that has an index template with no blocks in it (netiher single blocks, nor template parts) will not register as a block theme. Am I mistaken?

@costdev
3 years ago

Use str_contains() instead of parse_blocks().

#27 follow-up: @Ov3rfly
3 years ago

str_contains() is PHP8+ and will cause fatal error with lower PHP.

#28 follow-up: @costdev
3 years ago

@Ov3rfly str_contains() was polyfilled in WordPress 5.9.

#29 in reply to: ↑ 27 @antonvlasenko
3 years ago

Replying to Ov3rfly:

str_contains() is PHP8+ and will cause fatal error with lower PHP.

It will not cause fatal error. There is a polyfill version of that function.

#30 follow-up: @costdev
3 years ago

@andraganescu That's correct. A theme with a templates/index.html file that is either empty or contains non-block HTML will not be identified as a block theme. This is a possible regression - how likely is it that a block theme's templates/index.html would be empty or have no single blocks/template parts? - Genuine question.

In addition, a concern raised by @hellofromTonya:

Misidentified hybrid theme as a block theme, ie a theme that has classic HTML and templating but also could have blocks defined in them.

The hybrid theme issue above would not be a regression, as the mere existence of a templates/index.html with any amount of content would misidentify a hybrid theme as a block theme.

Given that, we can evaluate the concern, decide whether it needs to be handled and attempt to handle it in this ticket, but we can also adapt is_block_theme() to cover this later if we need more time.

Last edited 3 years ago by costdev (previous) (diff)

#31 in reply to: ↑ 28 @Ov3rfly
3 years ago

Replying to costdev:

Would recommend to use native available PHP functions as suggested in #23.

#32 @antonvlasenko
3 years ago

Replying to sabernhardt:

The block-based themes require a readable index.html file in both the 'block-templates' and 'templates' directories, so you apparently had the two directories.

index.html file has to be present in either block-templates or templates directory for a theme to be recognized as a block theme. It doesn't have to be present in both directories. Please correct me if I'm wrong.

Last edited 3 years ago by antonvlasenko (previous) (diff)

#33 @andraganescu
3 years ago

Random thought: I wonder if we should not assume that the block theme is the default kind of theme and check if the theme is a classic theme instead :) as in if not classic theme then it is a block theme. Is there something to check that a theme is definitely NOT a block theme?

how likely is it that a block theme's templates/index.html would be empty or have no single blocks/template parts?

@costdev no idea but likely or not, for developers it's going to cause some stackoverflow searches because it is unexpected to require some random contents just to be able to start with the theme, and switch to it :D

#34 @greenshady
3 years ago

This is a possible regression - how likely is it that a block theme's templates/index.html would be empty or have no single blocks/template parts? - Genuine question.

I am actually building a one-page theme and using front-page.html for the main/only template, which I thought made the most sense in this case. Currently, I have an index.html with the Post Content block in it for testing, but I had considered making this an empty template altogether. The reason I even have an index.html template there is because it is necessary to "register" it as a block theme with WP.

So, it's at least something I've thought about and a good question. It's not released yet, so I'll wait for this ticket to run its course before making any final decisions.

Ideally, we'd also have a "isBlockTheme":true flag in theme.json to specifically enable it instead of running file checks.

#35 @costdev
3 years ago

it is unexpected to require some random contents just to be able to start with the theme, and switch to it :D

Very important point. The theme author won't be able to start with a blank file and build it out in the Editor. As @youknowriad said:

the goal is make themes easy to create and also make them serializable (you can create without writing code)

So we would have to implement a way to tell the difference between an empty templates/index.html file in a classic/hybrid theme, and a block theme that is only starting development. Without being able to identify a theme as a skeleton theme, we would need a different approach here.

Regarding flipping the approach to non-block theme detection, I can't think of something that definitely won't be in a block theme, but definitely will be in classic/hybrid themes.

So, some thoughts:

  • Tag: full-site-editing exists for style.css. If a theme provides this, then WP_Theme::is_block_theme() should return true. While we're steering away from style.css as a requirement in the future, theme.json is currently optional.
  • If a theme uses theme.json, I can't think of an existing way to definitely identify a block theme. Maybe something like settings.isBlockTheme: true would be beneficial here.

I see @greenshady also mentioned an isBlockTheme flag in theme.json. I know we're trying to avoid asking theme authors to make a change to their themes, but at the same time, I wonder if we're at such an early stage of block themes, that changing this now will have the most minimal impact vs maybe needing it later.

#36 @antonvlasenko
3 years ago

I've created a flow chart describing how we could fix the WP_Theme::is_block_theme method. I've tried to take into account all the possible scenarios. But, of course, take it with a grain of salt.
https://cldup.com/THknG9F28v.png

Version 1, edited 3 years ago by antonvlasenko (previous) (next) (diff)

#37 @costdev
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

I have submitted a PR based on the paths outlined by @antonvlasenko. Not sure why it isn't appearing in Trac (yet).

Includes unit tests and ready for review.

#38 in reply to: ↑ 30 @manfcarlo
3 years ago

Here is another idea. Use scandir to check whether there are any non-html files in the templates folder, in addition to the existing check. In this case, the only false positives would occur if the theme has a templates folder with index.html and only other html files, which is not impossible but seems highly unlikely. It would be interesting to know if that would fix the false positive for @cantuaria your theme.

#39 follow-up: @ironprogrammer
3 years ago

I think @costdev's PR appropriately addresses the underlying issue, key to which is fixing compatibility with classic and hybrid themes that validly use (block-)templates/index.html files. (Thank you to everyone who has supplied input on the complexity here.)

As noted, `Tag: full-site-editing` already exists, so any future supports requirements will have this as an option, if needed.

But by not requiring explicit FSE registration, this update is consistent with the "no code" future envisioned for theme building, and accommodates old and new themes.

Kudos for the variations in the unit tests, as well a raising the issue on Gutenberg!

#40 in reply to: ↑ 39 @manfcarlo
3 years ago

Replying to ironprogrammer:

I think @costdev's PR appropriately addresses the underlying issue, key to which is fixing compatibility with classic and hybrid themes that validly use (block-)templates/index.html files. (Thank you to everyone who has supplied input on the complexity here.)

As noted, `Tag: full-site-editing` already exists, so any future supports requirements will have this as an option, if needed.

But by not requiring explicit FSE registration, this update is consistent with the "no code" future envisioned for theme building, and accommodates old and new themes.

Kudos for the variations in the unit tests, as well a raising the issue on Gutenberg!

I don't agree. As a plugin developer, I often look at the source code of core methods to ascertain what they are doing. This patch turns a fairly readable method into something that looks quite mystifying and difficult to make sense of. Admittedly, the flowchart helps somewhat, and could be linked in the documentation, but I'm not convinced that the complexity is unavoidable here.

I also think checking tags is a mistake. Tags are for compatibility with the theme directory, which not all themes need/want to be compatible with. It doesn't appear that a precedent exists in core, as case-sensitive searches through core files for ['Tags'] and ["Tags"] yielded no results. Plus, it duplicates exactly the same issue at hand, by assuming that any theme that happens to have a full-site-editing tag is a block theme.

It would not be correct to imply that this confusing flowchart-based approach is the only alternative to explicit registration. I would appreciate your consideration on my scandir suggestion above.

This ticket was mentioned in PR #2240 on WordPress/wordpress-develop by costdev.


3 years ago
#41

Based on the paths outlined by @anton-vlasenko. Includes unit tests. Ready for review.

Trac ticket: https://core.trac.wordpress.org/ticket/54910

#42 follow-up: @costdev
3 years ago

@ironprogrammer and @manfcarlo Thanks for your comments!

This patch turns a fairly readable method into something that looks quite mystifying and difficult to make sense of.

Can you identify the areas that are difficult to make sense of so that I can make it clearer?

It duplicates exactly the same issue at hand, by assuming that any theme that happens to have a full-site-editing tag is a block theme.

The Handbook entry for theme tags states:

Full Site Editing – support for block content areas (experimental)

and it links to the Handbook entry for Block themes.

The PR doesn't aim to require theme authors to add Tags: full-site-editing to their themes. It aims to take advantage of a theme that explicitly identifies itself as a block theme.

However, the patch is still a work in progress and the progress we've made means that the full-site-editing tag was, until now, the only way to identify a block theme when there is an empty (block-)templates/index.html file.

Use scandir to check whether there are any non-html files in the templates folder, in addition to the existing check.

I'm interested in possibly including this approach. Before I go and implement this and add unit test data, can we get some more opinions on this approach from both architectural and performance perspectives?

Note: Performance is a key concern here. If we do implement this and, in future, the list of valid index templates files includes other files in the (block-)templates trees, then at that point a $directories_to_skip mechanism should be included for this check. I can implement this now, but I'm hesitant to add code that doesn't apply to the files we're currently checking for.

In this case, the only false positives would occur if the theme has a templates folder with index.html and only other html files, which is not impossible but seems highly unlikely.

Absolutely, it is not impossible - but I agree that it's highly unlikely. I couldn't find any themes in the theme directory that have this, for example.

#43 @costdev
3 years ago

@Ov3rfly:

Would recommend to use native available PHP functions as suggested in #23.

Sorry, I just saw this comment.

While I typically agree to use native functions:

  • Allowing contributors to write code using modern PHP functions makes the contributor experience more consistent with modern development, and lowers the learning curve for newer developers.
  • As long as you evaluate the modern PHP function before polyfilling it, and you follow its spec when writing the polyfill, it should offer benefits in performance and/or stability and/or readability.
  • As we move to higher minimum PHP versions, the codebase remains intact except for updating wp-includes/compat.php.
  • We tested the polyfill extensively. That said, those interested in doing so can test it for discrepancies with the native str_contains() in PHP8+.

#44 in reply to: ↑ 42 @manfcarlo
3 years ago

Replying to costdev:

Can you identify the areas that are difficult to make sense of so that I can make it clearer?

Just the way that 5 different checks plus a caching mechanism are required, simply to determine whether a theme is a block theme or not. Intuitively, I would think it can't possibly be that hard.

My suggestion, which may not work, but assuming it does, would make for simpler and more readable code. It would only need to perform scandir, check whether all the file names end in .html, and check whether one of them is called index.html. It wouldn't need to parse blocks, check substrings, check the file content, check the theme tags or check for theme support.

I think you also misread my point about the full-site-editing tag, or maybe quoted the wrong part. In the part that you quoted, I was saying that if some hypothetical theme from 2012 happens to have the full-site-editing tag for some reason, maybe one that is outside of the theme directory, it would be interpreted as a block theme, despite being made 10 years before block themes were supported. That is not only unwanted, but the exact same problem as the current approach.

#45 @costdev
3 years ago

@manfcarlo Thanks for clarifying!

So:

  • we check the cache. If it exists, we return the value, saving resources.
  • we check that a block-templates/index.html or templates/index.html exists and is readable.
    • If none exists and is readable, we consider it a classic/hybrid theme.
    • For the index.html files that exist, we scan the directory for files that don't end in .html and are not .htaccess or web.config.
      • As soon as a 'disallowed' file is found, we skip to the next iteration/end the loop. If this happens for all possible index files, we consider it a classic/hybrid theme.
      • Otherwise, if no 'disallowed' files are found, we consider it a block theme.

Let's look at the case that opened this ticket: A templates/index.html file exists in a classic theme.

With the checks outlined above, this will be identified as a block theme only if index.html exists/is readable and there are no other files in the directory. The only time this is likely to occur is when the author has just created the directory.

I think it's safe to have a false positive in this instance until the theme author adds files to this directory, as it's likely a matter of seconds until a .php file is added.

For classic themes that use templating engines, these tend to be within inc, assets or other folders. I did a cursory scan for a common templating engine substring in the wp.org themes directory and reviewed the 40 themes that appeared in the results. None of them use the templates directory. For other scans, a templates directory was used, but didn't use .html files.

The one scenario I can think of is a newly initialized boilerplate where the additional files may not be added until later in the theme's development. However, I'd like to think that any boilerplate worth using would have more than just an index.html file in its templates folder. Given that, I think this implementation is safe enough in accuracy.

I've prepared a PR for this implementation, including unit tests, so that we can review and compare the implementations for accuracy and performance.

I'd appreciate everyone's opinions on this to see where we stand, as I'd like to see our agreed changes be considered for 5.9.1 if possible.

Last edited 3 years ago by costdev (previous) (diff)

This ticket was mentioned in PR #2247 on WordPress/wordpress-develop by costdev.


3 years ago
#46

This PR proposes that:

  • we check the cache. If it exists, we return the value, saving resources.
  • we check that a block-templates/index.html or templates/index.html exists and is readable.
    • If none exists and is readable, we consider it a classic/hybrid theme.
    • For the index.html files that exist, we scan the directory for files that don't end in .html and are not .htaccess or web.config.
      • As soon as a 'disallowed' file is found, we skip to the next iteration/end the loop.
      • Otherwise, if no 'disallowed' files are found, we consider it a block theme.

Trac ticket: https://core.trac.wordpress.org/ticket/54910

costdev commented on PR #2240:


3 years ago
#47

Removed the fix for get_query_template() as this turned out to be the fix for another ticket and is more appropriate to include there instead.

costdev commented on PR #2247:


3 years ago
#48

Removed the fix for get_query_template() as this turned out to be the fix for another ticket and is more appropriate to include there instead.

#49 @ironprogrammer
3 years ago

Thank you for continuing to explore this, @costdev and @manfcarlo.

While PR #2247 avoids parsing index.html, in favor of checking for other files while identifying block themes, it raises a couple of conerns:

  • In the context of the reported issue for this ticket -- having an existing templates/index.html file that doesn't contain blocks -- the theme is still incorrectly classified as FSE.
  • Going in the opposite direction, any implicit disallowed files would break FSE support. For instance, if an FSE theme included a .md or .txt reference file, or had a "shelved" HTML file, like .html.bak during development, as well as numerous other dotfiles that might sneak in. This could unintentionally hamper the FSE theme building workflow.

The implication with this approach is that block theme templates must ONLY have .html files in their directories, and that classic/hybrid themes must never do this. But as unconventional as it may appear, there are no rules against structuring a classic theme to use HTML in a templates folder, nor against plugins leveraging that folder for templated overrides built in HTML. These are only problematic now that FSE has come along with new assumptions.

Given the flexibility with theming afforded by WordPress, I'm not sure how we can know the intent of such files without parsing templates/index.html, when it exists. Since FSE themes do not need to "register" themselves, and there are no rules restricting subdirectories, it is up to WordPress to best understand their intent. The logic is slightly more complex, but PR #2240 is in a better position to make the determination as to whether the theme is intentionally targetting FSE.

#50 @costdev
3 years ago

@ironprogrammer I agree that PR 2247 relies too much on .html or other "allowed" files. I couldn't find any examples of a classic/hybrid theme using templates only for .html files, but I agree that it's possible and would misidentify these as block themes.

Regarding PR 2240, there's one thing I'd like to resolve, if possible.

An empty index.html file will only activate FSE when a full-site-editing tag is specified in the Tags header in style.css.

While an empty index.html may be seen as a rare event, as it would soon be populated with block markup, this will often not be the case.

For many (most?) authors, the index.html file will be empty until they go into the Editor, add some blocks, then copy and paste the markup into index.html. However, with PR 2240, they can't access the Editor until they add the full-site-editing tag.

This means a basic block theme that will primarily be developed within the Editor would be initialized as such:

style.css:

/**
 * Theme Name: My Block Theme
 * Tags: full-site-editing
 */

---

index.php:

 (blank)

---

templates/index.html:

 (blank)

This solution isn't ideal, as it effectively requires a block theme to register itself as one - something that's been avoided up until now. Some mix of parsing index.html and scandir would be too much of a performance hit IMO, even with caching.

Since the only two files required for all themes are index.php and style.css, I can't see a reliable way to identify an initialized block theme that has an empty index.html file without requiring some kind of 'registration' mechanism.

Can anyone see a way that a block theme might be identified based on something other than filenames/index.html contents, but without requiring a 'registration' mechanism?

Last edited 3 years ago by costdev (previous) (diff)

#51 @manfcarlo
3 years ago

Are block themes able to go without theme.json? If not, they could be identified by the existence of both templates/index.html and theme.json.

This ticket was mentioned in Slack in #core-editor by andraganescu. View the logs.


3 years ago

#53 @andraganescu
3 years ago

This really got complicated, so complicated that it needs a drawing to figure it out. There are currently under 50 block themes and we appear to try and fix a problem that block themes have by fixing how the system works instead of the problem at hand. The problem appears to be that that the original idea to consider a block theme any theme that has a file named "index.html" in a folder named "templates" was not bullet proof. Can't we fix that, I wonder, by:

  • asking the relatively, by comparison, few available block themes to explicitly declare themselves as block theme,
  • in a way which is compatible with our desire to make block themes "easy to be created and also make them serializable",
  • providing an escape hatch for users of block themes that don't update themselves?

I think that if we come up a good spec of how a block theme says "I am a block theme" and put up a dev note people will have time to update, as this whole new feature in WP is quite young. It is a better path than to surround one bad assumption with more relatively benign but potentially bad assumptions just to shield the original bad assumption.

Last edited 3 years ago by andraganescu (previous) (diff)

#54 @manfcarlo
3 years ago

I agree with @andraganescu above but I also have doubts about the lifetime of the block theme test. In the long run, I imagine it will be more sensible to hide/show different features based on the more specific checks, i.e. WP_Theme_JSON_Resolver::theme_has_support() for global styles and current_theme_supports( 'block-templates' ) for templates/parts/navigations, and phase out the use of wp_is_block_theme() altogether. Introducing a new declaration for themes should only be done if it's certain to be needed for the long term, and I think this was the reasoning for not introducing it thus far.

#55 @noisysocks
3 years ago

I think best to keep any solution here simple and then respond to further feedback as it arises.

The str_contains approach seems reasonable to me. Using parse_blocks will add an undesired performance hit and using scandir will not play nice with files such as .editorconfig, .DS_Store, etc.

If the issue is not particularly prevalent, then another option is to simply do nothing.

#56 @zieladam
3 years ago

I agree with @andraganescu, to quote 'The Zen of Python':

Explicit is better than implicit.
Simple is better than complex.

Inferring the information using a flowchart is implicit and complex, declaring it via a constant or theme.json entry is explicit and simple.

Last edited 3 years ago by zieladam (previous) (diff)

#57 follow-up: @andraganescu
3 years ago

There seems to be no consensus on how to move forward best. Recognizing @costdev 's effort to patch this ticket, and everyone else's input, should we just give this some more time to see if it is an actual problem? Maybe the number of times some classic theme is having this problem is extremely low and simply documenting the issue solves everything?

#58 in reply to: ↑ 57 @mcsf
3 years ago

Replying to andraganescu:

should we just give this some more time to see if it is an actual problem? Maybe the number of times some classic theme is having this problem is extremely low and simply documenting the issue solves everything?

Thanks for raising this question. Yes, my vote goes to giving this more time, especially given the impact of any of the proposed measures, and the uncertainty around affected sites and user expectations.

Note: See TracTickets for help on using tickets.