Convert theme.json to theme-json.php for better performance.

[spacedmonkey]

Trac ticket:

Fixes: WordPress/gutenberg#45616

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[peterwilsoncc]

This seems sensible, just a note about avoiding the need to commit additional built files.

[swissspidy]

From the Gutenberg issue:

The build process should render a PHP file returning an array with all the data from theme.json, including the relevant strings (based on theme-i18n.json) being wrapped in _x() calls.

This part is missing here.

[spacedmonkey]

This part is missing here.

@swissspidy I commented elsewhere. But converting that file doesn't work, as this is an object with object. So you to convert array of array to objects, which can be expensive. Do you think I should do that as well?

[swissspidy]

@spacedmonkey It's not about converting theme-i18n.json to PHP. That file can stay as-is.

It's about using _x() functions in theme-json.php, so that we don't even need to load theme-i18n.json anymore.

Let me explain:

With your PHP, theme-json.php is created that looks like this:

<?php 
// theme-json.php

return = [
   "version" => 2, 
   "settings" => [
      // ...
         "color" => [
               "background" => true, 
               "custom" => true, 
               "customDuotone" => true, 
               "customGradient" => true, 
               "defaultDuotone" => true, 
               "defaultGradients" => true, 
               "defaultPalette" => true, 
               "duotone" => [
                  [
                     "name" => "Dark grayscale", 
                     "colors" => [
                        "#000000", 
                        "#7f7f7f" 
                     ], 
                     "slug" => "dark-grayscale" 
                  ] 
               ] 
            ] 
      ]
      // ...
]; 

Then, WP_Theme_JSON_Resolver still calls static::translate() to translate it using the theme-i18n.json file.

That is slow.

So to fix this, the Grunt command should create the PHP file with translations automatically applied.

With the above example, it would look like:

<?php 
// theme-json.php

return = [
   "version" => 2, 
   "settings" => [
      // ...
         "color" => [
               "background" => true, 
               "custom" => true, 
               "customDuotone" => true, 
               "customGradient" => true, 
               "defaultDuotone" => true, 
               "defaultGradients" => true, 
               "defaultPalette" => true, 
               "duotone" => [
                  [
                     "name" => _x( "Dark grayscale", "Color name"), // See translation here
                     "colors" => [
                        "#000000", 
                        "#7f7f7f" 
                     ], 
                     "slug" => "dark-grayscale" 
                  ] 
               ] 
            ] 
      ] 
      // ...
]; 

[spacedmonkey]

@swissspidy

That is slow.

Is it, there profiling data for that? I feel like the I/O of file_get_content is much worse than loop around strings in PHP.

[swissspidy]

Is it, there profiling data for that? I feel like the I/O of file_get_content is much worse than loop around strings in PHP.

It is slower than not doing any file_get_contents at all. With the proposed change there is nothing to loop around in PHP by moving more stuff to the build process.

Hope my above example output makes sense.

[felixarntz]

+1 to @swissspidy's feedback, while this PR is a great start, it would be more efficient to have the _x() calls directly in the generated PHP file to not have to use theme-i18n.json (or a PHP equivalent) at all.

To be fair though, the existing blocks-json.php file in core also isn't doing that, it just has the en_US strings and translations are handled separately. So arguably what this PR here does is good enough for a first iteration. If we want to optimize to have the _x() / __() calls directly in the generated PHP file, it would apply to that existing core file as well, so not specific to the change here.

One question I have though, shouldn't this be implemented in Gutenberg first (hence I created it as a Gutenberg issue)? Or is there something that prevents us from doing that?

[kt-12]

Should we formally check if both JSON file are present before the readJSON operation? It shouldn't make much difference as we know files will be always present.

Also, should we add a verification task to check if the files were created. ref (verify:old-files).

	grunt.registerTask( 'verify:old-files', function() {
		const file = `${ BUILD_DIR }wp-admin/includes/update-core.php`;

		assert(
			fs.existsSync( file ),
			'The build/wp-admin/includes/update-core.php file does not exist.'
		);

[kt-12]

Thanks @spacedmonkey. This looks good to me just added a small question in Gruntfile.js

[peterwilsoncc]

@swissspidy @felixarntz Thanks for keeping the native english speakers honest.

This PR is probably a case in which it's best to let the perfect be the enemy of the good so the performance gains for en_US installs aren't considered enough and the rest of the world (including other en_* sites) left with the performance cost.

[felixarntz]

@peterwilsoncc I'm not sure I understand, are you saying we should hold off this change until we build in the translation support into the built file?

If I understand you correctly, you're concerned this change would only benefit en_US sites. That's not the case though, it would be a performance enhancement for any WordPress site already in its current shape. It simply avoids parsing the JSON file. The translation behavior wouldn't change from how it is today. So there are only wins here, no losses for anyone.

[spacedmonkey]

I don't think it makes sense to have some way of automated the merging and automatic translations of these two files. I think it is would hard to build and not make sense when this code is written. Translations can have complexities, like needing context parameters or plurral etc. Automating this conversation doesn't make sense to me.

I think there is a two courses of action to go forward.

1. We remain theme.json / theme-i18n.json, on build convert to php. As translate_settings_using_i18n_schema to merge the field from the two files together. As this PR does currently.
2. Create a theme-json.php file manually, including translations etc. Deprecate the use of theme.json / theme-i18n.json.

I also don't really understand why this is a json file at all. The point of json, is so that could be read outside of PHP, in javascript etc. But this file is only ever used in PHP.

I would like the thoughts of @peterwilsoncc @swissspidy @felixarntz on this one.

[swissspidy]

I don't think it makes sense to have some way of automated the merging and automatic translations of these two files.

But we already do that though. The merging is already automated, just at runtime (in translate_settings_using_i18n_schema() instead of build time. So we already deal with all these complexities—it's just a matter of porting over that logic. I don't see an issue with doing this 🤔

We remain theme.json / theme-i18n.json, on build convert to php. As translate_settings_using_i18n_schema to merge the field from the two files together. As this PR does currently.

This is a good start, but we can go even further. If we also automate the i18n stuff, we don't even need to read the theme-i18n file and don't even need to call translate_settings_using_i18n_schema() in most cases (i.e. if it's not a block theme) — making things faster.

Create a theme-json.php file manually, including translations etc. Deprecate the use of theme.json / theme-i18n.json.

That only works for the wp-includes/theme.json file, but themes can have theme.json files too that still need to be translated. For those, theme-i18n.json is still needed. So that file cannot be deprecated.

---

While I am writing all this I noticed that we should really do this for block-i18n.json and blocks-json.php as well!