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

Autoloader: use the wp-textdomain npm package to update package textdomains #18003

Closed
wants to merge 1 commit into from

Conversation

kbrown9
Copy link
Member

@kbrown9 kbrown9 commented Dec 7, 2020

We need to provide a way for a consuming plugin to use its own textdomain in the package strings. This PR presents a possible solution which uses the wp-textdomain NPM package to detect translatable strings and change them to a given textdomain.

How a Consumer Plugin Would Use This

  1. Add the jetpack-autoloader package to the plugin's composer dependencies in composer.json. (All plugins that use Jetpack packages should use the autoloader package to load package files.)

  2. Add a composer script to the plugins's composer.json file, with the plugin's textdomain as the first argument:
    "post-autoload-dump": "node vendor/automattic/jetpack-autoloader/js/update_textdomain.js {textdomain}"

  3. Add wp-textdomain as a development dependency in the plugin's package.json file.

  4. When the plugin is built with composer install, composer update or composer dump-autoload, the post autoload dump script will run. The script will update every textdomain in the .php files in the Automattic packages.

Changes proposed in this Pull Request:

  • Add wp-textdomain to Jetpack's development dependencies in package.json.

  • Add a new file to the autoloader package, update_textdomain.js, which uses wp-textdomain to detect and update the textdomain of package strings.

Why is this in the autoloader package?

I put this in the autoloader package because:

  • Every plugin that consumes Jetpack packages should use Jetpack's autoloader.
  • This code could (maybe should?) live in its own package, but then consumer plugins will need to include both the autoloader and new textdomain packages.
  • The autoloader is already used to build the plugin's composer package manifest files when a consuming plugin is built. Since the textdomains are changed when the plugin is built, the autoloader package seemed like a natural place to include this code.

Jetpack product discussion

  • n/a

Does this pull request change what data or activity we track or use?

  • no

Testing instructions:

  1. Add the composer script to composer.json. Enter a textdomain that's not jetpack so you can verify that the script worked.
    "post-autoload-dump": "node vendor/automattic/jetpack-autoloader/js/update_textdomain.js {textdomain}"

  2. Run composer dump-autoload. The textdomain for the translatable strings in the composer packages should be changed from jetpack to the string used in step 1.

Proposed changelog entry for your changes:

  • n/a

@kbrown9 kbrown9 self-assigned this Dec 7, 2020
@github-actions github-actions bot added the [Status] Needs Package Release This PR made changes to a package. Let's update that package now. label Dec 7, 2020
@jetpackbot
Copy link

jetpackbot commented Dec 7, 2020

Scheduled Jetpack release: January 12, 2021.
Scheduled code freeze: January 4, 2021

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.

Generated by 🚫 dangerJS against 7f10c5e

@matticbot
Copy link
Contributor

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 and confirm D54127-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@kbrown9 kbrown9 force-pushed the try/update-textdomain branch 3 times, most recently from 98b4be3 to eaeb10f Compare December 10, 2020 22:46
@kbrown9 kbrown9 force-pushed the try/update-textdomain branch from eaeb10f to 7f10c5e Compare December 11, 2020 15:19
@kbrown9
Copy link
Member Author

kbrown9 commented Dec 11, 2020

I'm closing this in favor of PR #18070, which will update the package documentation to provide information about updating textdomains. We've decided that the consumer plugins should implement the tools and processes that fit their needs.

@kbrown9 kbrown9 closed this Dec 11, 2020
@kraftbj kraftbj deleted the try/update-textdomain branch January 4, 2021 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE don't merge it! [Focus] Jetpack DNA [Package] Autoloader [Pri] Normal [Status] Needs Package Release This PR made changes to a package. Let's update that package now. [Status] Proposal Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants