(Try) Template Loader: Allow PHP in block templates and parts #25316
Conversation
|
Size Change: 0 B Total Size: 1.2 MB ℹ️ View Unchanged
|
|
For the record, I've been testing this using the seedlet-blocks theme modified as followed: Added to file block-templates-parts/header.htmlAdded to file block-templates/index.html |
|
Some observations that I shared in private:
|
|
I have to admit I kinda like this 😆 Perhaps we should add a new function? /**
* Get the contents of a template, executing any logic inside it.
*
* @param string $path Absolute path, or a file-path relative to the theme's root.
* @return string
*/
function get_template_part_contents( $path ) {
// Make it work both with absolute paths and theme-relative paths.
$path = file_exists( $path ) ? $path : get_theme_file_path( $path );
// Early exit if the file doesn't exist.
if ( ! file_exists( $path ) {
return '';
}
// Return the file contents.
ob_start();
include $path;
return ob_get_clean();
}In wp-core that function would be in |
|
Is there something blocking us from further pursuing this? 🤔 |
|
👋
Mainly, my own time and mental space to think this through vs. any other items that might be more important for WP5.6. :) |
Once the pattern is validated, sure, but I wouldn't put the cart before the horse! |
mcsf
requested review from
ajitbohra,
Soean,
spacedmonkey,
talldan and
TimothyBJacobs
as code owners
lib/template-loader.php
Outdated
| @@ -176,7 +176,9 @@ function create_auto_draft_for_template_part_block( $block ) { | |||
| } | |||
|
|
|||
| if ( $template_part_file_path ) { | |||
| $file_contents = file_get_contents( $template_part_file_path ); | |||
| ob_start(); | |||
| include $template_part_file_path; | |||
There was a problem hiding this comment.
Is there a way to "limit" the php functions that are available in the templates?
There was a problem hiding this comment.
I guess we could "exec php something" and just load the functions we want there but not sure how good is that :P
There was a problem hiding this comment.
Out of curiosity, why limit the functions that are available?
There was a problem hiding this comment.
Because we lose the declarativeness of these templates, their results might start to depend on very different things (database...) and the site editor might not reflect what the user would expect.
I'd rather start very small in what we allow and open later rather than doing the opposite (which is harder).
I think accessing i18n functions and assets are the main identified use-cases for me.
There was a problem hiding this comment.
I do consider that we are talking about two different things here.
Not necessarily... The way I see it we have 2 options if we want to resolve the translations issue:
- Implement a custom translation system for templates (something like
{{ __( 'Translate me', 'textdomain' ) }}) - Allow the use of PHP translations in the
.htmlfiles (<?php esc_html_e( 'Translate me', 'textdomain' ); ?>
This PR is an implementation of the 2nd method. That implementation coincidentally also solves an issue that - looking ahead - themes will have. Sure, it's not an issue now, because there are no FSE themes, but it will be once the big players start migrating their themes to FSE.
The suggestion above (only allow translation functions) will close the door to the happy side-effect that this PR has. Sure, it's not the intention of this PR to open up the system, but it does. Yes, there will always be those who write bad code. That's not something we can avoid. But dowe really want to limit the implementation to everyone in order to avoid the few people who will not play by what we consider to be best practices?
WordPress should be open, and allowing the system to be modified sounds more important than keeping the purity of templates that 3rd-party authors write.
The current state of this PR doesn't urge authors to do things they're not "supposed to" do with the vision we have in our heads about what the templating system should look like. In fact, most authors won't ever need translations in their templates or even references to images in the theme. 99% of the time, it will be simply copy-paste from the editor. But that 1% when we need something more, we should be able to do it.
There was a problem hiding this comment.
I don't think there is a need for this. PHP is already allowed in block templates, because blocks are allowed in PHP templates.
<?php
get_header();
do_blocks( '<!-- wp:site-title /-->\n<!-- wp:query -->\n<!-- /wp:query -->' );
get_footer();
The only real difference is the block templates don't require <!DOCTYPE html><head><?php wp_head(); ?></head> and the like. Other than that, it's just two ways to do the same thing and would be confusing for theme developers.
There clearly needs to be a solution for translations and assets but I think converting old themes to FSE is better achieved by rendering blocks in the PHP templates.
There was a problem hiding this comment.
I've thought a bit more about this, and beyond all the issues relating to placing PHP in FSE templates, there's another downside here.
And that is that if a user edits a template in the Site Editor, they will edit the computation of the PHP code. So whether it's the case of the conditional above, or the translation function, the result of the code will be in the template, and not the code itself.
Related to translations, there are existing solutions like Weglot that rely on DOM parsing to extract strings, and make them translatable. The advantage with Gutenberg is that the content (in block form) is already parsed. So there's no extra step needed to find out what can be translated or not.
So all that is needed is a way for theme authors to mark content as translatable, and there's no need to rely on PHP. What's more is that since Gutenberg knows about the context in which strings are stored, user provided translations can be auto-escaped.
That would relieve theme authors from having to do this manually. Which is a nice change, because plenty of existing templating engines both in PHP and JS have this built-in.
There was a problem hiding this comment.
So whether it's the case of the conditional above, or the translation function, the result of the code will be in the template, and not the code itself.
I think that's fine and the way it should be... If the user edits a template it gets stored as "static" content in the db and their edits should persist so no need for anything dynamic
There was a problem hiding this comment.
I also hope that the theme-mods things etc are on the way out and soon to be a thing of the past. But hoping is not the same as saying to all theme-developers who have spent a decade that a decade of their work is now useless.
There needs to be a plan.
If we don't allow logic in templates, then we must at least add filters to allow […]
Your defense of theme authors is definitely in the right spirit, but I do disagree with "their work is now useless". Traditional themes aren't being phased out, and theme authors and theme users alike can rest assured that what they have now will continue to work and suit their current needs.
The other idea that I need to articulate is this: FSE-ready block-based themes aren't just a technological jump. To use recent tech events as a simile, it's not like moving an ecosystem from x86 to ARM architecture. When switching architectures, compilers will perform different work to generate different app binaries, there will be significant performance benefits, but nothing really changes in how the apps behave, nor will there be changes in their UIs or the APIs they may use. In contrast, traditional WP themes and block-based themes are fundamentally different products as far as the user (not visitors, but site admins) is concerned. We can't just "emulate" traditional themes on the FSE "runtime" like we can emulate x86 programs. The use of conditional layouts and get_theme_mod are just excellent illustrations of this: the concepts don't directly map to the user-first site-editing experience that we strive for with FSE.
Thus, we have a duty to keep supporting traditional themes, but it's not our responsibility to automatically port them to the new system. They are different products with different trade-offs. Theme authors interested in porting their themes to blocks will decide, based on the affordances of FSE, what works best for them. In the meantime, we get to fine-tune the best interfaces to solve the underlying issues that e.g. conditional layouts and get_theme_mod sought to address in the first place. :)
gziolo
added
[Type] Technical Prototype
youknowriad
force-pushed
the
try/php-block-templates
branch
from
465a38c to
e54b597
Compare
| * @package gutenberg | ||
| */ | ||
|
|
||
| // Load the L10n library here. |
There was a problem hiding this comment.
I was not sure how to access these functions here.
We'd also need to validate $argv[1] here and makes sure it's file under the block-templates or template parts folder.
There was a problem hiding this comment.
This could be used to execute arbitrary files on the server, it's a major security hole.
Even with whitelisting, people could still hit this file directly in the browser to output the original templates and bypass what the user has set, actings as a means of exposing unintended data as well as other methods of exploiting the feature
| * | ||
| * @return array $block_template_files A list of paths to all template files. | ||
| */ | ||
| function gutenberg_get_template_paths() { |
There was a problem hiding this comment.
I think this function is useless now.
There was a problem hiding this comment.
Thanks for pushing that render-script experiment, @youknowriad!
| * @param string $template_path Template file path. | ||
| */ | ||
| function _gutenberg_read_template( $template_path ) { | ||
| return shell_exec( 'php ' . __DIR__ . '/render-script.php ' . $template_path ); |
There was a problem hiding this comment.
We should ensure that the new process uses the current PHP interpreter, not just the system's default php. We can do this with the PHP_BINARY constant as of 5.4:
| return shell_exec( 'php ' . __DIR__ . '/render-script.php ' . $template_path ); | |
| return shell_exec( PHP_BINARY . ' ' . __DIR__ . '/render-script.php ' . $template_path ); |
There was a problem hiding this comment.
I have deep misgivings about this approach of executing a new PHP process, it's encouraging developers to bootstrap WordPress to gain access to WP API functions, making these files extremely fragile and easy to break. This also means a big performance hit.
There are also servers that don't allow shell_exec for security reasons
There was a problem hiding this comment.
I had a thought, aside from a whitelist, it would be good to add 2 extra checks:
- A check to confirm this is being ran directly rather than via a browser request
- A nonce to confirm that it is WordPress running the file and not some other piece of code
It may also be worth looking into using an additional parameter to override ini values and prevent the use of networking and file writing functions
| // Subtract ending '.php'. | ||
| -4 |
There was a problem hiding this comment.
Could we program this? e.g.
$suffix = '.php';
$suffix_length = strlen( $suffix );
// ...
strpos( $path, $..., -$suffix_length );| @@ -44,9 +44,9 @@ function render_block_core_template_part( $attributes ) { | |||
| } else { | |||
| // Else, if the template part was provided by the active theme, | |||
| // render the corresponding file content. | |||
| $template_part_file_path = get_stylesheet_directory() . '/block-template-parts/' . $attributes['slug'] . '.html'; | |||
| $template_part_file_path = get_stylesheet_directory() . '/block-template-parts/' . $attributes['slug'] . '.php'; | |||
There was a problem hiding this comment.
Generalising template and template-part file access would be good, so that we don't depend on updating these paths and suffixes in multiple places.
|
This PR currently uses At the moment this severely limits the utility of the feature while introducing security, compatibility, and performance issues. E.g. if I have a theme with an image and want to include this in the block template, the code is forced to bootstrap WordPress to get to the |
|
The main idea is that if we allow any PHP code in the templates (access to the database...), block-based templates kind of lose their purpose which is to match the editor output and be editable there. We don't want to open the pandora box and only allow access to a given set of WordPress utils, granted that it's always possible to hack access to any API there. The idea too is that translation functions are available by default there, you don't need to write any code (it's not the case now) |
That would require loading a minimal subset of WordPress core, otherwise all APIs would be available. Plugins and themes would also need to be loaded so that filters and hooks work as well, otherwise things won't work as intended. At which point the whole of WordPress needs to be loaded. E.g. translations wouldn't work if In all honesty, with this implementation, I can see everybody moving all their exiting PHP templates one folder down, sticking a WP bootstrap at the top of the file, and calling their theme FSE ready. After all all the incentives are aligned that way. Even in an FSE first theme, bootstrapping WP in an FSE PHP template part is much easier to new developers than building blocks. Throw in a simple plugin to prevent the creation of FSE template posts in the database and hey presto. I know that's not what you intended, but I that is the natural consequence of this. As an aside, any guesses on how long it takes someone to put a JS snippet with a jQuery AJAX call to an |
Yes, that's the idea.
Why? We only care about translations for now and I'm ok starting with no translation filters personally. The idea is to try and discover the drawbacks first.
I don't think so personally because there's no point to have the site editor in that case and since the customizer and widgets are disabled, this is just useless. This is more about showing people what's good to do and what's not. We can just replace
I agree with that sentiment, PHP templates are the pandora box and if we go with them, the only good solution is to tell them what's good and what's bad. (their content won't update dynamically anyway) |
Do you have an idea of how to do this? I know there's the
I agree, it's a far from optimal situation to put yourself in, users do silly and unexpected things :)
If the runkit PHP extension were available we'd be able to have our cake and eat it without changes to WP Core by sandboxing the code properly |
|
Should this one be closed, or rebased to fix the merge conflicts? |
I'm not sure that we have any common solution based on all the good feedback. I'm fine closing the PR unless someone would like to take over. |
Addresses #21932.
Addresses #20966.
In particular, see comments #21932 (comment) and #20966 (comment).
Experimental PR. It's rudimentary, but it works.
Description
How has this been tested?
Screenshots
Types of changes
Checklist: