Make WordPress Core

Opened 17 months ago

Last modified 44 hours ago

#54797 assigned defect (bug)

Allow languages path in register_block_type

Reported by: rahe's profile Rahe Owned by: gziolo's profile gziolo
Milestone: 6.3 Priority: normal
Severity: normal Version: 5.5
Component: I18N Keywords: has-patch needs-refresh needs-unit-tests
Focuses: Cc:

Description

Hello,

On register_block_type and block.json right now the languages folder path is not handled.
This calls the register_block_script_handle function that just do :

<?php
        if ( ! empty( $metadata['textdomain'] ) && in_array( 'wp-i18n', $script_dependencies, true ) ) {
                wp_set_script_translations( $script_handle, $metadata['textdomain'] );
        }

Since it's pretty common to change where the languages are stored into a folder, I think we need to :

  • Add to the block.json an entry to specify the languages folder
  • Impact this part of code and add maybe add filters

Nicolas,

Attachments (2)

54797.diff (1.2 KB) - added by gziolo 16 months ago.
54797-domainpath.diff (1.2 KB) - added by gziolo 16 months ago.
Domain Path handling in block.json

Download all attachments as: .zip

Change History (26)

#1 @gziolo
17 months ago

We discussed this issue also on WordPress Slack (link requires registration at https://make.wordpress.org/chat):

https://wordpress.slack.com/archives/C02QB2JS7/p1641920214075100

There is also an issue opened in the Gutenberg project:

https://github.com/WordPress/gutenberg/issues/34478

There are two issues identified:

  1. When providing file paths to scripts (editorScript, script or viewScript), when there is a trailing ./ included then there is wrong md5 generated for the file and it doesn't match with the file generated in the translations folder.
  2. When a plugin provides a custom path to the translations folder, example: languages, it doesn't get passed to wp_set_script_translations called inside the register_block_script_handle helper function.

The first issue can be solved by always removing the trailing ./ from the file path passed in block.json.

The second issue can be tackled in many ways. One possibility is what @Rahe proposed in this ticket. An alernative to consider is to detect whether a script is registered inside a plugin (vs core block) and pass the custom path based on the setting read from the domain path set in the plugin metadata. More about that in https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/#domain-path.

This ticket was mentioned in Slack in #core-editor by gziolo. View the logs.


17 months ago

#3 @gziolo
16 months ago

  • Owner set to gziolo
  • Status changed from new to assigned

An alernative to consider is to detect whether a script is registered inside a plugin (vs core block) and pass the custom path based on the setting read from the domain path set in the plugin metadata. More about that in https://developer.wordpress.org/plugins/internationalization/how-to-internationalize-your-plugin/#domain-path.

It might be challenging to read the domain path provided in the plugin's metadata because get_plugin_data() is defined only in the admin but not on the frontend.

@gziolo
16 months ago

#4 @gziolo
16 months ago

  • Keywords has-patch has-unit-tests added
  • Milestone changed from Awaiting Review to 5.9.1
  • Type changed from feature request to defect (bug)

When providing file paths to scripts (editorScript, script or viewScript), when there is a trailing ./ included then there is wrong md5 generated for the file and it doesn't match with the file generated in the translations folder.

This part is ready with the patch attached.

#5 @Rahe
16 months ago

Hello,

Great!
A suggestion maybe can we just use ltrim('./', $path); instead of substr+strpos ?

For the plugin/theme header, this is read in the frontoffice too for the "classic" translations no ?

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


16 months ago

#7 @audrasjb
16 months ago

  • Milestone changed from 5.9.1 to 6.0

Hello, it looks like the issue wasn't introduced in 5.9 development cycle, so let's move this ticket to milestone 6.0 as 5.9.1 is dedicated to fix regressions reported against 5.9 :)

#8 @gziolo
16 months ago

I considered ltrim('./', $path); as a first option, but the way it works is a bit confusing. It removes all matching characters from the beginning of the string rather than the exact match we want here ./. Example: ./../file.js becomes file.jswhen you run the following code:

<?php
echo ltrim( './../file.js', './');

#9 @gziolo
16 months ago

  • Version changed from 5.9 to trunk

#10 @gziolo
16 months ago

In 52699:

I18n: Standardize the script paths for blocks

When providing file paths to scripts (editorScript, script or viewScript), when there is a trailing ./ included then there was a different md5 generated for the file that didn't match the one used with the file generated in the translations folder.

Props Rahe.
See #54797.

This ticket was mentioned in Slack in #core-i18n by gziolo. View the logs.


16 months ago

#12 @gziolo
16 months ago

When a plugin provides a custom path to the translations folder, example: languages, it doesn't get passed to wp_set_script_translations called inside the register_block_script_handle helper function.

I added a new patch that allows setting the equivalent of Domain Path like for the plugin. Exactly the same rules apply:

The domain path defines the location for a plugin’s translation. This has a few uses, notably so that WordPress knows where to find the translation even when the plugin is disabled. This defaults to the folder in which your plugin is found.

For example, if the translation is located in a folder called languages within your plugin, then the Domain Path is /languages and must be written with the first slash.

block.json

"textdomain": "my-plugin",
"domainpath" "/languages",

We will have to include similar documentation in Gutenberg in https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md with the note that this API is available starting from WordPress 6.0.

This ticket was mentioned in Slack in #core-i18n by gziolo. View the logs.


16 months ago

@gziolo
16 months ago

Domain Path handling in block.json

#14 @ocean90
15 months ago

#53097 was marked as a duplicate.

This ticket was mentioned in Slack in #core-editor by gziolo. View the logs.


15 months ago

#16 @costdev
14 months ago

  • Version changed from trunk to 5.5

#17 @costdev
13 months ago

  • Milestone changed from 6.0 to 6.1

With 6.0 RC1 starting soon, I'm moving this to the 6.1 milestone.

This ticket was mentioned in Slack in #cli by rahe. View the logs.


10 months ago

#19 @audrasjb
8 months ago

  • Milestone changed from 6.1 to 6.2

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

#20 @felipeelia
7 months ago

Hi there!

What I'm sharing here is the result of some time debugging a problem that although not strictly the same as outlined in the ticket seems to be so related that doesn't worth a new ticket IMHO. Let me know if you all prefer it in a new ticket and I can create that. I also want to ask you to excuse me if I missed something obvious here :)

I got here trying to translate a JS file added via block.json's editorScript. In my case, it has a long relative path, like file:/../../../../../dist/js/facets-meta-block-script.js

That seems to be generating a discrepancy between the md5 value while creating the .json file and the md5 value actually expected when loading translations. Here is some code to (hopefully) clarify it a bit:

File creation

It seems to use the "real" file name here. In this case, dist/js/facets-meta-block-script.js.

File detection

Due to the way we are generating the src value here, it becomes https://<domain>/wp-content/plugins/elasticpress/assets/js/blocks/facets/taxonomy/../../../../../dist/js/facets-block-script.js. That assets/js/blocks/facets/taxonomy is where the block.json is located.

Then in load_script_textdomain, the value used to get an md5 hash here is actually assets/js/blocks/facets/meta/../../../../../dist/js/facets-meta-block-script.js and not dist/js/facets-meta-block-script.js as used during the generation step.

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


4 months ago

This ticket was mentioned in Slack in #core by costdev. View the logs.


3 months ago

#23 @swissspidy
3 months ago

  • Milestone changed from 6.2 to 6.3

#24 @swissspidy
44 hours ago

  • Keywords needs-refresh needs-unit-tests added; has-unit-tests removed

Patch needs an update to pass an empty string instead of null by default. Also needs unit tests.

Note: See TracTickets for help on using tickets.