In progress - Config: Add a class that customizes the textdomain in packages #14650

kbrown9 · 2020-02-12T07:39:30Z

This is a POC to explore a possible solution for using textdomains in the packages. All of the implementation details are up for debate, but I think the general idea works.

The Consuming Plugin

The consuming plugin must set two items in its composer.json file:

the post-autoload-dump script in the scripts section.
the textdomain in the extra section.

For example, the scripts and extra sections of Jetpack’s composer.json file would look like:

"scripts": {
		"php:compatibility": "vendor/bin/phpcs -p -s --runtime-set testVersion '5.6-' --standard=PHPCompatibilityWP --ignore=docker,tools,tests,node_modules,vendor --extensions=php",
		"php:lint": "vendor/bin/phpcs -p -s",
		"php:changed": "vendor/sirbrillig/phpcs-changed/bin/phpcs-changed --git",
		"php:autofix": "vendor/bin/phpcbf",
		"php:lint:errors": "vendor/bin/phpcs -p -s --runtime-set ignore_warnings_on_exit 1"
		"php:lint:errors": "vendor/bin/phpcs -p -s --runtime-set ignore_warnings_on_exit 1",
		"post-autoload-dump": "Automattic\\Jetpack\\Config\\Textdomain_Customizer::post_autoload_dump"
	},
"extra": {
	"textdomain": "jetpack"
},

The Jetpack Packages

The Jetpack packages must provide a list of their translatable files in the extra section of their composer.json files. This PR includes a temporary example file in the Config package. The Config package composer.json includes this extra section listing the example file:

"extra": {
		"translatable": [
			"/automattic/jetpack-config/src/example-file.php"
		]
	}

How it works

When composer dump-autoload is run (it's run after both install and update), the Textdomain_Customizer::post_autoload_dump() method is called at the end of the dump-autoload process.
The version of the Config package is checked. If it’s a development version, the post_autoload_dump() method just returns. This prevents the files from being changed and causing headaches during development.
The plugin's composer.json file is checked for a textdomain entry in the extra section. If it exists, that textdomain will be used in the package files. If it does not exist, 'default' will be used.
The textdomain_customizer traverses through the installed composer packages, looking for Jetpack packages and the translatable entry in each Jetpack package's composer.json file. If a Jetpack package has a translatable entry, the listed files are processed.
The files are processed by replacing every occurrence of the placeholder JETPACK_CUSTOMIZE_TEXTDOMAIN with either the plugin's textdomain or 'default'.

Changes proposed in this Pull Request:

Add a new class and method, Textdomain_Customizer, that customizes the textdomain of strings in the packages. I put this in the Config class because it might be useful for multiple packages.
Update Jetpack's composer.json file with the post_autoload_dump method and Jetpack's textdomain.
Added a unit test. I need to write more tests.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This is a new feature.

Testing instructions

Checkout this branch.
Run composer install and check the example file included in this PR, packages/config/src/example-file.php. The textdomain in this file should still be the placeholder JETPACK_CUSTOMIZE_TEXTDOMAIN because you're using a development version of the Config package.
To test that the method works, comment out this line to skip the early return for development versions.
Run composer install and check file packages/config/src/example-file.php. The textdomain should be 'jetpack'.

Proposed changelog entry for your changes:

tbd

To do

A couple of things need to be done:

Refactoring to make CodeClimate happy.
More unit tests.
Display error messages when expected files don't exist.

matticbot · 2020-02-12T07:39:36Z

Caution: This PR has changes that must be merged to WordPress.com
Hello kbrown9! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D38741-code before merging this PR. Thank you!

jetpackbot · 2020-02-12T07:41:24Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against 9a935a6

kraftbj · 2020-02-12T19:45:36Z

This is nice. From a quick read through, I ask default becomes default-textdomain or something that is more unique before this concept gets to a merge-ready state. I wouldn't want future us being really confused when get_option( 'something', 'default'); starts returning 'jetpack' in production.

kbrown9 · 2020-02-12T19:59:37Z

This is nice. From a quick read through, I ask default becomes default-textdomain or something that is more unique before this concept gets to a merge-ready state. I wouldn't want future us being really confused when get_option( 'something', 'default'); starts returning 'jetpack' in production.

Yes, definitely. I used 'default' so that there would be a valid textdomain if the consuming plugin doesn't provide one in composer.json. A better approach would be to use something unique, as you suggested, and then change it to 'default' when a textdomain isn't provided.

packages/config/src/class-textdomain-customizer.php

Add a new class, Textdomain_Customizer, that customizes the textdomain of strings in the packages. It uses the textdomain given in the extra section of the plugin's composer.json file. Update Jetpack's composer.json file with the post_install method and Jetpack's textdomain. Provide a preliminary unit test for the new class.

matticbot · 2020-02-14T07:09:15Z

kbrown9, Your synced wpcom patch D38741-code has been updated.

kbrown9 · 2020-02-14T07:44:41Z

I made a lot of changes to this, so I force-pushed a new commit.

I used a more unique placeholder, JETPACK_CUSTOMIZE_TEXTDOMAIN.
The package files with strings now need to be identified in each package's composer.json file.
The Textdomain_Customizer has been totally refactored to support the new way that the package files are identified.

A couple of things need to be done:

The tests are failing because I used an anonymous class in a unit test. (Oops - I thought those were added in PHP 5.6.)
It looks like I have to do a bit more refactoring to make CodeClimate happy.
I added one unit test, and I plan to write more.

zinigor

I love where this is going! This will solve the i18n problem inside packages thanks to how flexible it promises to be! I left a couple of comments, but nothing major.

zinigor · 2020-02-14T17:11:27Z

packages/config/src/class-textdomain-customizer.php

+		}
+
+		foreach ( $files as $file ) {
+			$file_path = $this->vendor_dir . $file;


I tried googling for best practices of combining path segments into a single path in PHP, and threw up a little :)
Let's at least do trailingslashit for vendor_dir and realpath for the entire thing?

This class is used when composer install, composer update, or composer dump-autoload are run, so trailingslashit isn’t available. Maybe we can just use rtrim() to remove any trailing slashes, and then add one?

I’ll also use realpath() on the file paths.

zinigor · 2020-02-14T17:14:11Z

packages/config/src/class-textdomain-customizer.php

+			return;
+		}
+
+		$file_contents = file_get_contents( $file_path );


Do you think loading the whole file into memory is going to be scalable? I don't know how to do it line by line yet, I'm sure it's going to be more fuss, do you think it's worth looking into?

That’s a good point, and I’ll look into it.

Actually, it's going to be a lot of fuss with all the file renaming, etc. Let's just keep what we have here and add error handling for file not being writable.

packages/config/src/class-textdomain-customizer.php

…ests

packages/config/src/class-textdomain-customizer.php

zinigor · 2020-02-18T14:11:03Z

packages/config/src/class-textdomain-customizer.php

+		foreach ( $files as $file ) {
+			$file_path = $this->vendor_dir . $file;
+
+			if ( ! file_exists( $file_path ) ) {


We have three different places here that check for either file_exists or is_file, maybe that all can be moved to customize_textdomain_in_file? This will also help with the "cognitive complexity" in CodeClimate analysis.

…paths Use rtrim() to remove trailing slashes and then add a slash to $vendor_dir. This ensures that $vendor_dir has at least on trailing slash in case $file doesn't have a starting slash. Use realpath() to prepare the file paths. Refactor customize_textdomain_in_files() to separate the recursive directory work into its own method. This should reduce the cognitive complexity a bit.

stale · 2020-05-19T05:03:53Z

This PR has been marked as stale. This happened because:

It has been inactive in the past 3 months.
It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

kbrown9 · 2020-12-07T22:25:38Z

I'm closing this in favor of the approach introduced in PR #18003. The approach described in this PR is too prone to errors. For example, it would be easy for developers to forget to use the textdomain placeholder when adding strings to the packages. The approach introduced in PR #18003 is much easier to work with.

kbrown9 added [Status] In Progress [Pri] Normal [Focus] Jetpack DNA labels Feb 12, 2020

kbrown9 requested a review from a team February 12, 2020 07:39

kbrown9 self-assigned this Feb 12, 2020

matticbot added the Touches WP.com Files label Feb 12, 2020

github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Feb 12, 2020

kbrown9 force-pushed the try/use_composer_extra_replace_textdomain branch from fc76d84 to adae740 Compare February 12, 2020 18:51

kbrown9 requested a review from zinigor February 12, 2020 19:08

zinigor reviewed Feb 12, 2020

View reviewed changes

packages/config/src/class-textdomain-customizer.php Outdated Show resolved Hide resolved

kbrown9 force-pushed the try/use_composer_extra_replace_textdomain branch from adae740 to 8bec3d0 Compare February 14, 2020 07:09

zinigor reviewed Feb 14, 2020

View reviewed changes

kbrown9 added 2 commits February 14, 2020 14:04

Confg: create a minimal package class for the textdomain_customizer t…

e5998e8

…ests

Config Textdomain_Customizer: change placeholder textdomain

acb669b

zinigor reviewed Feb 18, 2020

View reviewed changes

packages/config/src/class-textdomain-customizer.php Show resolved Hide resolved

zinigor reviewed Feb 18, 2020

View reviewed changes

kbrown9 mentioned this pull request Apr 28, 2020

Move connection owner admin notice #15568

Open

stale bot added the [Status] Stale label May 19, 2020

stale bot removed the [Status] Stale label Dec 7, 2020

matticbot removed the Touches WP.com Files label Dec 7, 2020

kbrown9 closed this Dec 7, 2020

kraftbj removed the [Status] In Progress label Oct 8, 2021

kraftbj deleted the try/use_composer_extra_replace_textdomain branch April 17, 2025 13:24

In progress - Config: Add a class that customizes the textdomain in packages #14650

In progress - Config: Add a class that customizes the textdomain in packages #14650

Uh oh!

Conversation

kbrown9 commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The Consuming Plugin

The Jetpack Packages

How it works

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions

Proposed changelog entry for your changes:

To do

Uh oh!

matticbot commented Feb 12, 2020

Uh oh!

jetpackbot commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kraftbj commented Feb 12, 2020

Uh oh!

kbrown9 commented Feb 12, 2020

Uh oh!

Uh oh!

matticbot commented Feb 14, 2020

Uh oh!

kbrown9 commented Feb 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zinigor left a comment

Choose a reason for hiding this comment

Uh oh!

zinigor Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

kbrown9 Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

zinigor Feb 14, 2020

Choose a reason for hiding this comment

Uh oh!

kbrown9 Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

zinigor Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zinigor Feb 18, 2020

Choose a reason for hiding this comment

Uh oh!

stale bot commented May 19, 2020

Uh oh!

kbrown9 commented Dec 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kbrown9 commented Feb 12, 2020 •

edited

Loading

jetpackbot commented Feb 12, 2020 •

edited

Loading

kbrown9 commented Feb 14, 2020 •

edited

Loading