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

I18n messages from Composer packages are never translated #21690

Closed
anomiex opened this issue Nov 9, 2021 · 12 comments · Fixed by #22151
Closed

I18n messages from Composer packages are never translated #21690

anomiex opened this issue Nov 9, 2021 · 12 comments · Fixed by #22151
Assignees
Labels
[Focus] i18n Internationalization / i18n, adaptation to different languages [Package] Autoloader [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. [Plugin] Boost A feature to speed up the site and improve performance. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Search A plugin to add an instant search modal to your site to help visitors find content faster. [Pri] High [Type] Bug When a feature is broken and / or not performing as intended

Comments

@anomiex
Copy link
Contributor

anomiex commented Nov 9, 2021

Impacted plugin

Jetpack, Backup, Boost

Steps to Reproduce

Easiest way to observe this at the moment is to compare the list of strings available for translation for the Backup plugin at https://translate.wordpress.org/projects/wp-plugins/jetpack-backup/dev/es/default/ with the strings that are present in packages/backup.

Or you could run wp i18n make-pot on one of the plugins and look more directly.

A clear and concise description of what you expected to happen.

All the strings from the package are able to be translated, somehow.

What actually happened

Only the ones defined in the plugin itself can be translated.

Other information

The problem is that wp i18n make-pot ignores vendor/.

There was some discussion at p1634933470116600-slack-CBG1CP4EN.

One idea would be to configure composer to use something other than vendor/ for the plugins that bundle vendor/. But it was said that this would need some work in the Jetpack Autoloader package.

Another idea would be to somehow get the packages translated as standalone projects, and then either bundle the translations in the packages or somehow convince WordPress to download the packages' i18n data along with the plugin. There doesn't seem to be a straightforward way to do this at the moment.

Operating System

Other

OS Version

No response

Browser

Other / Not applicable

Browser Version(s)

No response

@anomiex anomiex added [Type] Bug When a feature is broken and / or not performing as intended [Focus] i18n Internationalization / i18n, adaptation to different languages [Package] Autoloader [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. [Plugin] Boost A feature to speed up the site and improve performance. [Plugin] Search A plugin to add an instant search modal to your site to help visitors find content faster. labels Nov 9, 2021
@jsnmoon
Copy link
Contributor

jsnmoon commented Nov 9, 2021

This issue will most likely block @Automattic/jetpack-search from moving forward with our code migration to the standalone package. Does @Automattic/jetpack-crew have an idea on when/if this issue will be addressed?

For context, we're aiming to complete our code migration to the standalone package by the end of this month.

cc @bluefuton

@kangzj
Copy link
Contributor

kangzj commented Nov 11, 2021

Probably still a work around. Just a thought here, we could possibly publish the JS builds to _inc/build, i.e. folders that make-pot doesn't ignore and git wouldn't version by using a env variable or sth. For PHP, the same thing.

@jeherve
Copy link
Member

jeherve commented Nov 11, 2021

Only the ones defined in the plugin itself can be translated.

Is the problem linked to the files in /vendor, or the files using a textdomain (jetpack) that is not the plugin's defined textdomain, as discussed in #20320 and #18070?

One idea would be to configure composer to use something other than vendor/ for the plugins that bundle vendor/.

If /vendor is indeed excluded, would it be an option to suggest that it stop being excluded?

@kraftbj
Copy link
Contributor

kraftbj commented Nov 11, 2021

vendor is being excluded by Core's make-pot. IMO, pushing Core/WP CLI to not exclude vendor for the plugins directory is the most stable way forward.

@anomiex Where do things stand on needing a dynamic textdomain (e.g. what textdomain is used for a package that is in both Jetpack-the-plugin and a standalone)? Whether or not vendor is excluded, IIRC, we'll run into issues if there is a different textdomain.

@kraftbj
Copy link
Contributor

kraftbj commented Nov 11, 2021

On the WP CLI side, the default exclusion of vendor was last brought up (that I could see) in wp-cli/i18n-command#172

If WP CLI does not want to entertain a change to the default, we could open a meta.trac ticket regarding .org including vendor (or perhaps having some way for a plugin to indicate to include vendor, or a specific folder string like vendor/automattic/ , etc

@kraftbj
Copy link
Contributor

kraftbj commented Nov 11, 2021

Lastly, going back to how we did JS translations could work. On build, we make-pot ourselves, convert that to an array in PHP that has the plugin textdomain and include that.

@jeherve
Copy link
Member

jeherve commented Nov 11, 2021

On build, we make-pot ourselves, convert that to an array in PHP that has the plugin textdomain and include that.

I was happy when we were able to move away from this towards core js translations, but I can see how useful it could be here. Such a system may allow us to address both this issue and #20320 at once, when building the production version of a plugin generated from the monorepo.

@kraftbj
Copy link
Contributor

kraftbj commented Nov 11, 2021

I was happy when we were able to move away from this towards core js translations, but I can see how useful it could be here.

Yeah, I really don't like having to go back to a homegrown solution, but given that the solutions otherwise are to get Core or WP CLI to move on something when we're looking at wanting something ready to go into prod at the end of month... I think homegrown may be the best way forward until the ecosystem (with our help) can have a solution.

@anomiex
Copy link
Contributor Author

anomiex commented Nov 11, 2021

I'm supposed to be AFK today, but I was thinking about this anyway and had a few ideas.

  • A composer custom installer could put just the relevant packages outside of vendor/, assuming we're willing to identify them all with an appropriate "type" field in their composer.json files. That might avoid the trouble with the autoloader since all the metadata would still be in vendor/.
  • It looks like we could get away with not actually mangling the domains in the packages.
    • When WordPress.org's infrastructure calls wp i18n make-pot, it passes the --ignore-domain option. So the strings will get picked up regardless of the domain the package uses, all will be placed in the plugin's pot file (so under the plugin's domain).
    • The trick then would be to somehow map all the packages' domains to the plugin's domain, so when the package does __( 'some test', 'package-domain' ) that it loads from the plugin's domain instead. I don't see any direct way to alias domains in WordPress, there's no filter in the get_translations_for_domain() code path, but we could use the gettext, ngettext, gettext_with_context, and ngettext_with_context filters to make it happen for each message. The gettext filter function might look something like
      public function gettext_filter_alias( $translation, $text, $domain ) {
          if ( $translation === $text && isset( $this->aliases[$domain] ) ) {
              return translate( $text, $this->aliases[$domain] );
          }
          return $translation;
      }
      The others would be similar. For the JS, we could use load_script_translation_file to swap the domains in the filename.
    • If all that works, then all we need is to build the alias map. We could have the autoloader collect the list of packages' domains from the packages' composer.jsons, then the plugin would use that to register the alises.

Thoughts?


Longer term, if we could get WordPress.org to change some of their infrastructure, something like this might make sense:

  1. Switch the infrastructure from being based on plugins and themes (which all use the plugin or theme slug as the text domain) to being based on the text domains directly. For example, instead of https://api.wordpress.org/translations/plugins/1.0/ and https://api.wordpress.org/translations/themes/1.0/ just have one endpoint that takes the domain.
  2. Let code declare to WordPress which domains it needs beyond the defaults of "plugin slug" and "theme slug". This is so WordPress can download those extra domains, I don't think WordPress cares beyond that.
  3. Let us register projects that aren't plugins or themes (e.g. our packages) on translate.wordpress.org, so the packages can be translated and WordPress can fetch those translations like it does plugins and themes.

That way all we'd need to do would be to have the plugin declare the packages' text domains, and the translators would only have to translate each package's strings once instead of for every plugin using the package.

@kbrown9
Copy link
Member

kbrown9 commented Nov 15, 2021

A composer custom installer could put just the relevant packages outside of vendor/, assuming we're willing to identify them all with an appropriate "type" field in their composer.json files. That might avoid the trouble with the autoloader since all the metadata would still be in vendor/.

Your ideas seem reasonable to me.

Do you think there's any value in also investigating how the autoloader would need to be changed to allow composer to be configured to use something other than the default vendor directory? For example, I know that the autoloader will check for the generated classmap in the vendor directory, so we would need to make sure to keep the classmap in vendor.

@anomiex
Copy link
Contributor Author

anomiex commented Nov 15, 2021

We could investigate it, but we shouldn't need to if my idea pans out. I suspect the best route if we did want to go in that direction would be to have it check for some specific non-vendor directory in addition to vendor/. Or, I guess, have the autoloader write vendor/ even if Composer uses something else.

@kbrown9
Copy link
Member

kbrown9 commented Nov 15, 2021

I suspect the best route if we did want to go in that direction would be to have it check for some specific non-vendor directory in addition to vendor/. Or, I guess, have the autoloader write vendor/ even if Composer uses something else.

If we did need to go in this direction, I think we would need to have the autoloader write some files to the vendor directory and have Composer use something else. This would be necessary for compatibility with the autoloader packages that are already in released plugins. Those autoloaders would be checking for the autoloader files in the other plugins' vendor directories. But, as you said, we probably won't need to go in this direction.

anomiex added a commit that referenced this issue Dec 2, 2021
This script is the one expected by the default configuration of the new
`@automattic/i18n-loader-webpack-plugin`.

Note the `domainMap` is currently empty. Code to fill that in will come
later, as part of #21690, once we figure out the exact form of it.
anomiex added a commit that referenced this issue Dec 9, 2021
This script is the one expected by the default configuration of the new
`@automattic/i18n-loader-webpack-plugin`.

Note the `domainMap` is currently empty. Code to fill that in will come
later, as part of #21690, once we figure out the exact form of it.
anomiex added a commit that referenced this issue Dec 9, 2021
This script is the one expected by the default configuration of the new
`@automattic/i18n-loader-webpack-plugin`.

Note the `domainMap` is currently empty. Code to fill that in will come
later, as part of #21690, once we figure out the exact form of it.
anomiex added a commit that referenced this issue Dec 13, 2021
This adds the ability to register textdomain aliases with the Assets
package.

Assets then hooks into `__()` and so on to have them check the
aliased-to domain if no translation is found for the aliased-from
domain. It also hooks into script translation file loading to load a
translation file for the aliased-to domain if no file exists for the
aliased-from domain.

This brings us one step closer to fixing #21690. Followups will switch
all the packages to use alias-able textdomains (instead of them all
using "jetpack") and arrange for the aliases to actually be registered.
anomiex added a commit that referenced this issue Dec 15, 2021
This adds the ability to register textdomain aliases with the Assets
package.

Assets then hooks into `__()` and so on to have them check the
aliased-to domain if no translation is found for the aliased-from
domain. It also hooks into script translation file loading to load a
translation file for the aliased-to domain if no file exists for the
aliased-from domain.

This brings us one step closer to fixing #21690. Followups will switch
all the packages to use alias-able textdomains (instead of them all
using "jetpack") and arrange for the aliases to actually be registered.
anomiex added a commit that referenced this issue Dec 22, 2021
To finally fix #21690, we need each package to have its own textdomain and
to declare that domain in its `composer.json`.

The work done in previous PRs will take it from there: composer-plugin will
include the packages' domains in the generated `i18n-map.php` (#22096),
which the plugin will pass to the Assets package (#22143), which will map
those package textdomains to the plugin's (#22083).

This also adds checks to the monorepo linting script to help people get the
setup right:

* Packages may not use a textdomain that matches a plugin slug.
* If the package's phpcs config or eslint config sets a textdomain (which it
  will have to if it uses i18n after #22117 and #22131), require that same
  textdomain is set in composer.json.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] i18n Internationalization / i18n, adaptation to different languages [Package] Autoloader [Plugin] Backup A plugin that allows users to save every change and get back online quickly with one-click restores. [Plugin] Boost A feature to speed up the site and improve performance. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Search A plugin to add an instant search modal to your site to help visitors find content faster. [Pri] High [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants