Add a script that creates a PHP file based on a POT by atimmer · Pull Request #5310 · WordPress/gutenberg

atimmer · 2018-02-28T15:43:10Z

Description

This is a workaround to make Gutenberg translatable. translate.wordpress.org ignores the POT file provided by the plugin and parser the PHP file itself. We can work around this by providing a PHP file generated on the POT file.

How Has This Been Tested?

I've copied the code from wordpress-seo and adapted it for a non-grunt environment. So the code has been tested by wordpress-seo. I've run it once to see that it generates something sensible.

The real tests can only be done once it is on translate.wordpress.org. Which this PR + a release will accomplish.

See #5169. Note that this PR is the short-term solution.

This is a workaround to make Gutenberg translatable.

youknowriad · 2018-02-28T15:45:40Z

bin/pot-to-php.js

@@ -0,0 +1,102 @@
+#!/usr/bin/env node


Should we run this script in ./bin/build-plugin-zip.sh?

I swear I commented before your commit :P

Yeah, just realized that myself. I have added it to there.

youknowriad

LGTM 👍

Might be good to have @aduth's opinion as well.

aduth · 2018-02-28T15:53:53Z

bin/build-plugin-zip.sh

 status "Generating translation messages..."
 npm run gettext-strings
+status "Generating PHP file for wordpress.org to parse translations..."
+npm run pot-to-php


Few lines below, we need to include this new PHP file in the generated ZIP archive.

aduth · 2018-02-28T15:56:40Z

bin/pot-to-php.js

+#!/usr/bin/env node
+
+const gettextParser = require( 'gettext-parser' );
+const _isEmpty = require( 'lodash/isEmpty' );


What's with the _ prefix?

Minor: Might be slightly more optimized this way, but in a Node context we don't care as much about granular module includes, and could inadvertently encourage future maintainers to add more Lodash dependencies in a similar fashion, whereas the following would be simple to include more utilities in batch fashion:

const { isEmpty } = require( 'lodash' );

We use the _ prefix to designate lodash functions. Will change.

aduth · 2018-02-28T15:57:28Z

bin/pot-to-php.js

+const path = require( 'path' );
+const fs = require( 'fs' );
+
+const TAB = '\t';


The value this constant provides is questionable 😄

I think I found the code easier to read after introducing these constants.

aduth · 2018-02-28T16:02:04Z

bin/pot-to-php.js

+	let original = translation.msgid;
+	const comments = translation.comments;
+
+	if ( ! _isEmpty( comments ) ) {


Pretty sure that while we're including the file reference in the PHP file, we're omitting the comments that would be picked up in the PHP string extraction, both text of the original comment and file reference.

https://codex.wordpress.org/I18n_for_WordPress_Developers#Descriptions

https://github.com/WordPress/wordpress-develop/blob/b15919c8dd7e4085b854b03a352db497a39f114a/tools/i18n/extract.php#L17

aduth · 2018-02-28T16:03:50Z

bin/pot-to-php.js

+	return php;
+}
+
+function convertPotToPHP( potFile, phpFile, options ) {


Mixed abbreviation capitalization. Should either be convertPotToPhp or convertPOTToPHP, my preference toward the latter.

atimmer · 2018-02-28T16:52:49Z

I've fixed most of the comments, @aduth.

In this paragraph:

Pretty sure that while we're including the file reference in the PHP file, we're omitting the comments that would be picked up in the PHP string extraction, both text of the original comment and file reference.

What do you mean with file reference? I put the file references in their prefixed with // Reference:.

aduth · 2018-02-28T18:07:18Z

For the purposes of string extraction, "// Reference: " is pretty useless. They're not picked up as translator comments for GlotPress.

See also: https://www.gnu.org/software/gettext/manual/html_node/PO-Files.html

Specifically:

#. extracted-comments
#: reference…

aduth · 2018-02-28T18:09:16Z

I guess for the automatic string extraction, it's going to automatically assign the PHP file as reference? In which case if a human were to follow it to the file, the "Reference:" comment to the JavaScript source could be valuable.

We still want the text of the comments represented via /* translators: ... */ though

aduth

Aside from one note, this is looking pretty great 👍

aduth · 2018-02-28T23:26:24Z

bin/pot-to-php.js

+		original = escapeSingleQuotes( original );
+
+		if ( isEmpty( translation.msgid_plural ) ) {
+			php += TAB + `__( '${ original }', '${ textdomain }' )`;


We also need:

_x (determined by translation.msgctxt)

_nx (as _nx_noop, determined by translation.msgctxt, translation.msgid_plural)

atimmer · 2018-03-01T14:20:58Z

I don't think there is a way to tell the PHP parser that the reference to the file should be different. I think it would be confusing if we put that in the translator comment. So we have to make do with translators going to the PHP file then discovering that they need to go to the JS file. #5169 is the correct solution for this.

I will address _x and _nx.

This will generate _x and _nx when appropriate.

atimmer · 2018-03-01T14:48:04Z

I have addressed your last point @aduth. In the process, I also found #5330.

aduth

Looks like recent changes might have broken the formatting of the PHP output. Reference comments are no longer on their own lines.

aduth · 2018-03-01T16:23:45Z

bin/pot-to-php.js

@@ -0,0 +1,121 @@
+#!/usr/bin/env node


If we drop this and instead call from the npm script as node ./bin/pot-to-php.js, wouldn't we have the advantage of added Windows support?

Sorry, for some reason email notifications for code reviews were turned off.

I can definitely see this change. I copied it from ./bin/create-php-parser.js. After this is merged I can create a PR that addresses both files.

atimmer · 2018-03-02T09:07:01Z

Fixed the remaining issue.

pento

This is such a wonderful hack, I love it. 😁

Obviously we need a long term solution, but I'm fine with this for now.

Add a script that creates a PHP file based on a POT

c04b82a

This is a workaround to make Gutenberg translatable.

atimmer requested review from aduth and youknowriad February 28, 2018 15:43

Add POT to PHP to plugin ZIP build

c5bfa0d

youknowriad reviewed Feb 28, 2018

View reviewed changes

atimmer added this to the 2.3 milestone Feb 28, 2018

atimmer added the [Type] Build Tooling Issues or PRs related to build tooling label Feb 28, 2018

youknowriad approved these changes Feb 28, 2018

View reviewed changes

aduth requested changes Feb 28, 2018

View reviewed changes

atimmer added 2 commits February 28, 2018 17:47

Include PHP file in build zip

a3bf3df

Drop _ prefix before isEmpty

196ad55

aduth requested changes Feb 28, 2018

View reviewed changes

Take context into account for PHP file

408b473

This will generate _x and _nx when appropriate.

atimmer mentioned this pull request Mar 1, 2018

Fix extraction of _nx translation function #5330

Merged

3 tasks

aduth reviewed Mar 1, 2018

View reviewed changes

aduth added the Internationalization (i18n) Issues or PRs related to internationalization efforts label Mar 1, 2018

Fix NEWLINE alignment issue of translations

8ac8f45

pento approved these changes Mar 2, 2018

View reviewed changes

atimmer merged commit 6098d12 into master Mar 2, 2018

atimmer deleted the add/pot-to-php-conversion branch March 2, 2018 12:15

youknowriad mentioned this pull request Mar 6, 2018

i18n: Merge PHP and JS strings in POT generation #5169

Closed

felixarntz mentioned this pull request Dec 21, 2021

Ensure that module header fields can be translated WordPress/performance#60

Merged

Conversation

atimmer commented Feb 28, 2018

Description

How Has This Been Tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atimmer commented Feb 28, 2018

Uh oh!

aduth commented Feb 28, 2018

Uh oh!

aduth commented Feb 28, 2018

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atimmer commented Mar 1, 2018

Uh oh!

atimmer commented Mar 1, 2018

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atimmer commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pento left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

atimmer commented Mar 2, 2018 •

edited

Loading