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

make-pot: scan any theme.json file in any level #424

Merged
merged 5 commits into from
Dec 20, 2024
Merged

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented Dec 19, 2024

  • scan any theme.json file in any level, for any type of project
  • only scan style variations in a top-level /styles folder if it is a theme (this restriction is new)

Fixes #423

@swissspidy swissspidy added this to the 2.6.4 milestone Dec 19, 2024
@swissspidy swissspidy requested a review from a team as a code owner December 19, 2024 16:24
Signed-off-by: Pascal Birchler <pascalb@google.com>
Signed-off-by: Pascal Birchler <pascalb@google.com>
@swissspidy
Copy link
Member Author

@oandregal @ocean90 would appreciate your feedback on this.

@oandregal
Copy link
Contributor

I've looked at the command used to extract core strings. It creates a few different files:

  • wordpress-continents-cities.pot: no strings coming from theme.json are related to this.
  • wordpress.pot: this looks like the strings for the front-end? No strings coming from theme.json need to be displayed in the front-end.
  • hello-dolly.pot: no strings coming from theme.json are related to the hello-dolly plugin.
  • wordpress-admin.pot: strings to load on the admin. The strings coming from theme.json need to be available in all block editors. If this is the file we load for that, they need to be loaded here.
  • wordpress-admin-network.pot : this sounds like the admin network screen. No strings coming from theme.json here.

I suppose we should exclude wp-content from all those contexts, apart for some minor tweaks (include dolly, etc.). This would suffice to prevent theme-defined strings (be in PHP like patterns or functions.php, or any of the theme.json (main or style variations) from being picked for the core pot.

With the limited context I have about how the extraction works, I believe the i18n-command should never skip a path. It's the consumer responsibility to configure the command properly and they can already --exclude-* paths and --skip-* types of files (PHP, JSON, etc.).

Signed-off-by: Pascal Birchler <pascalb@google.com>
Copy link
Contributor

@oandregal oandregal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This direction sounds good to me, thanks for the quick fix.

I've shared an edge case that we may want to address before merging.

@swissspidy
Copy link
Member Author

Thanks @oandregal for your help and thinking of all possible edge cases here 🦸 🚀

@wp-cli/committers if someone could give this a quick review & approval, that would be greatly appreciated!

@swissspidy swissspidy merged commit bf6720f into main Dec 20, 2024
46 checks passed
@swissspidy swissspidy deleted the fix/423-themejson branch December 20, 2024 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix theme.json string extraction
3 participants