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

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

Merged
merged 6 commits into from Mar 2, 2018

Conversation

Projects
None yet
4 participants
@atimmer
Member

atimmer commented Feb 28, 2018

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.

Add a script that creates a PHP file based on a POT
This is a workaround to make Gutenberg translatable.

@atimmer atimmer requested review from youknowriad and aduth Feb 28, 2018

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

This comment has been minimized.

@youknowriad

youknowriad Feb 28, 2018

Contributor

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

This comment has been minimized.

@youknowriad

youknowriad Feb 28, 2018

Contributor

I swear I commented before your commit :P

This comment has been minimized.

@atimmer

atimmer Feb 28, 2018

Member

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

@atimmer atimmer added this to the 2.3 milestone Feb 28, 2018

@youknowriad

LGTM 👍

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

@@ -81,6 +81,8 @@ status "Generating build..."
npm run build
status "Generating translation messages..."
npm run gettext-strings
status "Generating PHP file for wordpress.org to parse translations..."
npm run pot-to-php

This comment has been minimized.

@aduth

aduth Feb 28, 2018

Member

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

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

This comment has been minimized.

@aduth

aduth Feb 28, 2018

Member

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' );

This comment has been minimized.

@atimmer

atimmer Feb 28, 2018

Member

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

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

This comment has been minimized.

@aduth

aduth Feb 28, 2018

Member

The value this constant provides is questionable 😄

This comment has been minimized.

@atimmer

atimmer Feb 28, 2018

Member

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

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

This comment has been minimized.

@aduth

aduth Feb 28, 2018

Member

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

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

This comment has been minimized.

@aduth

aduth Feb 28, 2018

Member

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

atimmer added some commits Feb 28, 2018

@atimmer

This comment has been minimized.

Member

atimmer commented Feb 28, 2018

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

This comment has been minimized.

Member

aduth commented Feb 28, 2018

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

This comment has been minimized.

Member

aduth commented Feb 28, 2018

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 👍

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

This comment has been minimized.

@aduth

aduth Feb 28, 2018

Member

We also need:

  • _x (determined by translation.msgctxt)
  • _nx (as _nx_noop, determined by translation.msgctxt, translation.msgid_plural)
@atimmer

This comment has been minimized.

Member

atimmer commented Mar 1, 2018

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.

Take context into account for PHP file
This will generate _x and _nx when appropriate.

@atimmer atimmer referenced this pull request Mar 1, 2018

Merged

Fix extraction of _nx translation function #5330

0 of 3 tasks complete
@atimmer

This comment has been minimized.

Member

atimmer commented Mar 1, 2018

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.

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

This comment has been minimized.

@aduth

aduth Mar 1, 2018

Member

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?

This comment has been minimized.

@atimmer

atimmer Mar 2, 2018

Member

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.

@aduth aduth added the i18n label Mar 1, 2018

@atimmer

This comment has been minimized.

Member

atimmer commented Mar 2, 2018

Fixed the remaining issue.

@pento

pento approved these changes Mar 2, 2018

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

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

@atimmer atimmer merged commit 6098d12 into master Mar 2, 2018

2 checks passed

codecov/project 39.62% (-0.04%) compared to 4004ee2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@atimmer atimmer deleted the add/pot-to-php-conversion branch Mar 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment