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

Blocks: Add i18n support to register_block_type_from_metadata #868

Closed
wants to merge 5 commits into from
Closed

Blocks: Add i18n support to register_block_type_from_metadata #868

wants to merge 5 commits into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jan 13, 2021

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

Related issue in Gutenberg: WordPress/gutenberg#23636.

Related PR in WP-CLI from @swissspidy: wp-cli/i18n-command#210.

Description

There are some issues related to internationalization (i18n) support in the introduced block.json metadata files that still need to be resolved.

There is a proposal in block registration document that we still need to implement:
https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/block-api/block-metadata.md#internationalization-not-implemented

The good news is that WP-CLI has implemented (wp-cli/i18n-command#210) native support for translatable strings extraction from block.json file in i18n command. It was based on the aforementioned document.


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.

@ocean90
Copy link
Member

ocean90 commented Jan 13, 2021

I'm missing the part where the text domain is passed to register_block_script_handle() so that wp_set_script_translations() can be called. Is that still being added?

package-lock.json Outdated Show resolved Hide resolved
@gziolo
Copy link
Member Author

gziolo commented Jan 13, 2021

I'm missing the part where the text domain is passed to register_block_script_handle() so that wp_set_script_translations() can be called. Is that still being added?

Right, I think I missed it. I saw something similar added to Create Block, but it wasn't backported to the core.

https://github.com/WordPress/gutenberg/blob/556505024833dd815dbc365e75c2279f003cd472/packages/create-block/lib/templates/esnext/%24slug.php.mustache#L45

return false;
}

if ( ! empty( $metadata['textdomain'] ) ) {
Copy link
Member Author

@gziolo gziolo Jan 14, 2021

Choose a reason for hiding this comment

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

@ocean90, is it good to assume that if there is no textdomain defined in block.json that this block/plugin isn't meant to be translated?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say so, yes. Would core blocks set it to default? But it actually doesn't seem like it will ever be empty due to the default being default?

$textdomain = isset( $metadata['textdomain'] ) ? $metadata['textdomain'] : 'default';

Copy link
Member Author

Choose a reason for hiding this comment

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

True, I thought I removed this line 😅
Yes, I believe we either explicitly set it to default for core blocks or auto-detect them through core namespace in the block name and auto-apply default this way.

Copy link
Member Author

@gziolo gziolo Jan 19, 2021

Choose a reason for hiding this comment

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

I removed this fallback for now as it's way more complicated for blocks than for plugins where we have:

    // If no text domain is defined fall back to the plugin slug.
    if ( ! $plugin_data['TextDomain'] ) {
        $plugin_slug = dirname( plugin_basename( $plugin_file ) );
        if ( '.' !== $plugin_slug && false === strpos( $plugin_slug, '/' ) ) {
            $plugin_data['TextDomain'] = $plugin_slug;
        }
    }

Plugins fall back to the plugin slug, so we could do the same. What do you think?

The challenges I see:

  • block.json can be located anywhere, so the parent folder might be not the best way to pick
  • block.json might be inside the plugin so it would have the textdomain define but the challenge is how to read it
  • block.json might be inside the theme so it would have the textdomain define but the challenge is how to read it
  • name field is constructed from namespace/slug so we could use it in a quite reliable way

@gziolo gziolo marked this pull request as ready for review January 14, 2021 17:48
@lukecarbis
Copy link

This PR will need eyes from the documentation team.

@hellofromtonya
Copy link
Contributor

@gziolo gziolo deleted the update/register-block-metadata-i18n branch January 23, 2021 18:23
@ZebulanStanphill
Copy link
Member

Shouldn't textdomain be textDomain to match the camelCase already being used for editorScript and editorStyle?

@gziolo
Copy link
Member Author

gziolo commented Jan 26, 2021

@ZebulanStanphill, I know it's confusing. In the written language it's two words so you would expect text_domain in PHP and textDomain in JavaScript, but in practice, it' was mostly textdomain in PHP codebase so it follows that. Related docs:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants