Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix theme.json string extraction #423

Open
swissspidy opened this issue Dec 18, 2024 · 6 comments · May be fixed by #424
Open

Fix theme.json string extraction #423

swissspidy opened this issue Dec 18, 2024 · 6 comments · May be fixed by #424

Comments

@swissspidy
Copy link
Member

In #320 we limited extraction of theme.json files to ones in the (theme) root and in a /styles folder.

We forgot the fact that there's wp-includes/theme.json in core and lib/theme.json in Gutenberg which are currently forgotten because of that, thus those strings aren't extracted nor translatable in these projects. But they should be!

We probably need to drop that limitation and just scan any theme.json file we encounter...

Meta changes (in sites/trunk/wordpress.org/public_html/wp-content/plugins/wporg-gp-customizations/inc/cli/class-make-core-pot.php) are probably required too once that lands. To avoid any regression there.

/cc @ocean90 @oandregal

@oandregal
Copy link
Contributor

Sharing this thread for reference.

@oandregal
Copy link
Contributor

oandregal commented Dec 19, 2024

WordPress 5.8 was released in July 2021. The theme.json strings for WordPress, Gutenberg, and themes were available in GlotPress in December 3rd, 2021. The code to extract them was part of i18n-command 2.2.9. The change in 320 was part of of i18n-command 2.4.0. Are we able to detect which version of WordPress first used i18n-command 2.4.0? That'd tell us when this started happening.

The reason I mention this is that I suppose we have to trigger the extracting string process for every version of WordPress starting in 5.8. Otherwise, the theme.json strings won't be there.

I don't know how the string extraction process works, so I can't speak more about the traceability. This is what I know: the strings for WordPress 5.8 were there when I checked and they are not now in 5.8. Something must have changed them post-release — after December 3rd, 2021 to be exact. I checked and the next events I see are:

Do we re-trigger the extraction process for every minor? Perhaps we used i18n-command 2.4.0 in 5.8.3? Just oversharing in case it's helpful.

@oandregal
Copy link
Contributor

oandregal commented Dec 19, 2024

@swissspidy looking at the current code, does this codepath receives all files of a given project? If so, can we just add two other checks so that lib/theme.json (gutenberg) or wp-includes/theme.json (wordpress-develop) are also processed?

You also mentioned the following:

That change was made with the assumption that theme.json is supposed to live only in themes and in the root folder of theme. This helped avoid an issue where, when extracting strings in core, theme.json strings from all the default themes were erroneously included (because they live in wp-content.

Don't we have the same issue with strings coming from PHP? Themes can have strings defined in their functions.php file, so how do we deal with those? I suppose there are various ways to make it work: running the string extraction process for core with an empty wp-content folder (or an ignore flag), telling the extractors (PHP, theme.json, etc.) to ignore anything coming from wp-content, adding an allowlist of files in the extractors (current approach for themejsonextractor). I don't know enough the specifics to suggest any approach, but wanted to raise it to make sure we consider a holistic solution.

@swissspidy
Copy link
Member Author

I don't want do add hardcoded file paths to the string extractor.

I think we just need to scan every theme.json file that exists, and make sure that dotorg is able to exclude the ones it doesn't need.

I suppose there are various ways to make it work: running the string extraction process for core with an empty wp-content folder (or an ignore flag), telling the extractors (PHP, theme.json, etc.) to ignore anything coming from wp-content, adding an allowlist of files in the extractors (current approach for themejsonextractor). I don't know enough the specifics to suggest any approach, but wanted to raise it to make sure we consider a holistic solution.

Thanks, I appreciate you thinking of all these possibilities!

As you can see at https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/plugins/wporg-gp-customizations/inc/cli/class-make-core-pot.php, Meta has multiple include/exclude configs for the three projects in use (dev, admin, continents & cities). Most likely a change to those configs would be needed.

The reason I mention this is that I suppose we have to trigger the extracting string process for every version of WordPress starting in 5.8. Otherwise, the theme.json strings won't be there.

I think that will be automated anyway whenever there is a new release. We don't need to know specific version numbers or dates.

@oandregal
Copy link
Contributor

As you can see at https://meta.trac.wordpress.org/browser/sites/trunk/wordpress.org/public_html/wp-content/plugins/wporg-gp-customizations/inc/cli/class-make-core-pot.php

I've been trying to access that link for the whole morning but I'm getting a 504 🤔

@swissspidy
Copy link
Member Author

@swissspidy swissspidy linked a pull request Dec 19, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants