WordPress Core support #69

swissspidy · 2018-08-05T15:12:14Z

Changes in this PR:

Allows passing more than one file to --merge
Does not error when no plugin or theme file has been found
Adds glob support to --exclude (see Ignore minified JavaScript files #45)
Adds a new --include argument as an addition to --exclude
Adds new --copyright-holder and --package-name arguments
These values are used for the copyright notice comment at the top of the POT file.
The naming of the arguments and the usefulness of that comment as a whole should be discussed.
Adds a new ~~--except~~ --subtract option that accepts one or more POT files
Strings that are on this "blacklist" won't be extracted

To do:

Add necessary features for supporting string extraction in WordPress core (and perhaps any other project)
Cover these new features in tests

The following commands can be used to generate the necessary POT files for WordPress core.

Note: the Report-Msgid-Bugs-To URL can be overridden using the --header argument.

Continents & Cities

wp i18n make-pot /path/to/wordpress/core /path/to/target/wp-tz.pot \
--include="wp-admin/includes/continents-cities.php" \
--package-name="WordPress" \
--copyright-holder="WordPress" \
--debug \
--skip-js \
--ignore-domain

WordPress (Development 4.9.x on translate.wordpress.org)

wp i18n make-pot /path/to/wordpress/core /path/to/target/wordpress.pot \
--exclude="wp-admin/*,wp-content/themes/*,wp-includes/class-pop3.php,wp-content/plugins/akismet/" \
--package-name="WordPress" \
--copyright-holder="WordPress" \
--debug \
--skip-js \
--ignore-domain

Administration

This one involves some more steps as it should include all files except for WordPress and Network Admin files. Also, the Hello Dolly plugin, which is bundled with WordPress core, needs to be translated as well.

Hello Dolly:

wp i18n make-pot /path/to/wordpress/core/wp-content/plugins /path/to/target/hello-dolly.pot \
--include="hello.php" \
--exclude="akismet" \
--debug \
--skip-js \
--ignore-domain

WordPress Administration

wp i18n make-pot /path/to/wordpress/core /path/to/target/wp-admin.pot \
--exclude="wp-admin/network/*,wp-admin/network.php,wp-admin/includes/class-wp-ms*,wp-admin/includes/network.php,wp-admin/includes/continents-cities.php" \
--include="wp-admin/*" \
--merge="/path/to/target/hello-dolly.pot" \
--subtract="/path/to/target/wp-frontend.pot" \
--package-name="WordPress" \
--copyright-holder="WordPress" \
--debug \
--skip-js \
--ignore-domain

Network Admin

wp i18n make-pot /path/to/wordpress/core /path/to/target/wp-network-admin.pot \
--include="wp-admin/network/*,wp-admin/network.php,wp-admin/includes/class-wp-ms*,wp-admin/includes/network.php" \
--subtract="/path/to/target/wp-frontend.pot,/path/to/target/wp-admin.pot" \
--package-name="WordPress" \
--copyright-holder="WordPress" \
--debug \
--skip-js \
--ignore-domain

Fixes #36.
Fixes #45.

See #36.

See #45.

Fixes #45.

swissspidy · 2018-08-05T17:49:26Z

Here's the output of the WP-CLI command and makepot.php for a quick comparison:

makepot.zip
wp-cli-i18n-command.zip

The POT files for continents and cities have the same number of strings (524). The others seem to differ quite a bit.

Tested both with current WordPress master/trunk

Type	WP-CLI	makepot.php
Frontend	2917	2665
Continents & Cities	524	524
Administration	2975	2645
Network Admin	405	311

I took the numbers on the right from translate.wordpress.org as the local output with makepot.php seems to be quite different. Not sure why though.

Perhaps @ocean90 can help me find the cause for this :-)

swissspidy · 2018-08-06T06:15:20Z

Instead of the nightly build I ran this now against 4.9.8 as downloaded straight from WordPress.org/download.

The numbers are much better now:

Type	WP-CLI	makepot.php
Frontend	2665	2665
Continents & Cities	524	524
Administration	2981	2645
Network Admin	404	311

Administration now includes Hello Dolly (as does makepot.php / translate.wordpress.org), that's why that number is higher than before.

swissspidy · 2018-08-06T06:53:50Z

So, the reason for the difference between the admin files seems to be that makepot.php removes any strings that are already part of the frontend POT file to avoid any duplicates.

It uses the msgcat utility for that. So when generating the admin POT file it actually does this:

Generate frontend POT file
Generate admin POT file
Merge frontend with admin file and only keep the common strings
Merge admin file with the common file and only keep the unique strings

I'm not sure this is something that could or should be done within this command.

I think if WordPress.org is going to use the WP-CLI command at some point for string extraction, they might add a little wrapper script around it to use msgcat as outlined above to keep existing behavior.

schlessera · 2018-08-06T07:17:11Z

Instead of using msgcat, maybe it would be worthwhile to add a --deduplicate flag to --merge ?

swissspidy · 2018-08-06T12:25:22Z

@schlessera So you'd pass a file name to --deduplicate, e.g. --deduplicate=wp-frontend.pot?

Then we could use something like this:

wp i18n make-pot /path/to/wordpress/core /path/to/target/wp-admin.pot \
--exclude="wp-admin/network/*,wp-admin/network.php,wp-admin/includes/class-wp-ms*,wp-admin/includes/network.php,wp-admin/includes/continents-cities.php" \
--include="wp-admin/*" \
--merge="/path/to/target/hello-dolly.pot" \
--deduplicate="/path/to/target/wp-frontend.pot" \
--package-name="WordPress" \
--copyright-holder="WordPress" \
--debug \
--skip-js \
--ignore-domain

schlessera · 2018-08-06T12:46:22Z

I'm not sure, I ignore how the files relate to each other in detail, and what will be considered the source of truth.

But generally, if you have a --merge command, it makes sense in my hand to specify how duplicates are handled.

schlessera · 2018-08-06T12:47:28Z

Hmm, I see the problem now. What you need is not deduplication, but set operations like union and most of all intersection.

swissspidy · 2018-08-06T13:30:00Z

I added an experimental --except argument now.

You can pass one or more exceptions. The command extracts all strings from them and compares them against the ones found by the command.

Strings that are contained in the exception list will be removed from final output.

This way, I got the resulting POT file for wp-admin down to 2733 strings (compared to 2981 before).

swissspidy · 2018-08-06T13:39:48Z

Alright, got it down to 2647! That's only 2 differences.

Note regarding exceptions: this might be useful as a new merge flag in the upstream gettext library instead, see https://github.com/oscarotero/Gettext#merge-translations.

swissspidy · 2018-08-06T13:45:24Z

I'm not sure why, but the output generated by makepot.php seems to be missing the following two strings:

<strong>ERROR</strong>: "Table Prefix" can only contain numbers, letters, and underscores.
source: wp-admin/setup-config.php:262
<strong>ERROR</strong>: "Table Prefix" must not be empty.
source: wp-admin/setup-config.php:258

I can't find these strings on https://translate.wordpress.org either.

Sounds like a bug to me and a reason why this command here should be used instead :-)

swissspidy · 2018-08-06T16:02:31Z

Updated and final numbers:

Type	WP-CLI	makepot.php
Frontend	2665	2665
Continents & Cities	524	524
Administration	2647	2645
Network Admin	311	311

I feel like now we only need to discuss the wording of the newly added arguments or, in the case of the copyright notice, the usefulness of these.

schlessera · 2018-08-06T19:14:07Z

Regarding the --except flag, I think the name is slightly misleading. I feel like --subtract might be clearer, wouldn't it?

schlessera · 2018-08-06T19:16:25Z

README.md

-	[--merge[=<file>]]
-		Existing POT file file whose content should be merged with the extracted strings.
+	[--merge[=<files>]]
+		One or more existing POT files whose contents should be merged with the extracted strings.


The syntax for multiple files should be documented.

Do you mean documented like <file1,file2> or as an example in the argument description?

Not sure what the best way is, as you have multiple arguments with the same issue.

Maybe a small adjective like comma-separated and then a general note in the command description might be clearer?

[--merge[=<file[,file2,...]>]] Comma-separated list of POT files whose contents should be merged with the extracted strings.

Something like that, maybe? Unsure about the notation...

Turns out something like [--merge[=<file[,file2,...]>]] isn't valid.

I'll stick with the description though.

schlessera · 2018-08-06T19:16:52Z

README.md

 		If left empty, defaults to the destination POT file.

+	[--except=<files>]


As already commented, I think --subtract would be a clearer name.
Also, syntax for multiples files should be documented.

schlessera · 2018-08-06T19:17:51Z

README.md

+	[--except=<files>]
+		If set, only strings not already existing in one of the passed POT files will be extracted.
+
+	[--include=<paths>]


Syntax for multiple paths needs to be documented.

schlessera · 2018-08-06T19:20:59Z

features/makepot.feature

+      """
+      <?php
+
+       __( 'Hello World' );


A regular project would normally use regular gettext with single underscore: _()

I can add both, although I don‘t think we should enphasise support for any regular, non-WP gettext project.

It was more about having an example that is technically correct. Outside of WordPress, __() would normally not exist.

schlessera · 2018-08-06T19:29:12Z

src/MakePotCommand.php

+		$this->slug             = Utils\get_flag_value( $assoc_args, 'slug', Utils\basename( $this->source ) );
+		$this->skip_js          = Utils\get_flag_value( $assoc_args, 'skip-js', $this->skip_js );
+		$this->headers          = Utils\get_flag_value( $assoc_args, 'headers', $this->headers );
+		$this->package_name     = Utils\get_flag_value( $assoc_args, 'package-name', 'Unknown' );


Shouldn't the package and copyright holder be optional?
If I understand correctly, this will now always be added, but with values of 'Unknown'.

We could either make it optional, change the wording (just „Copyright 2018“ without any author or name) or remove it completely. I‘m not sure if it really serves any purpose.

Also, two command arguments just for this comment is a bit too much I think. Perhaps we could go with a singular argument to manually override that line?

Yes, something more generic like --file-comment might be preferable.

See #47 (comment) and #55 for previous discussion about a --comment / --file-comment argument

I think --comment could be ambiguous, as it could just as well refer to a translator comment.

I added --file-comment but left package-name as it's needed/useful for the Project-Id-Version POT header.

swissspidy · 2018-08-06T19:30:49Z

It‘s certainly not ideal, yes. I have yet to come up with a better name. Subtract could work.

schlessera · 2018-08-06T21:30:20Z

🎉

johnbillion · 2018-11-09T19:39:22Z

@swissspidy Regarding the two "Table prefix" strings that makepot missed, it might be because they were malformed. See https://core.trac.wordpress.org/changeset/43205.

WordPress Core support

swissspidy added 7 commits August 5, 2018 17:04

Experiment with WordPress core support

296539c

See #36.

Simple glob support

fd434bf

See #45.

Fix wrong defaults

9a557e6

Ignore minified JS files

876c8a8

Fixes #45.

Update tests

97298c7

Fix directory exclusion

23dd034

Fix exclusion and inclusion once again...

4394737

Add option for string exceptions

d0d6cb4

No in-place array modifications

d7052f2

Fix debug output

84722d5

swissspidy changed the title ~~[WIP] Experiment with WordPress core support~~ WordPress Core support Aug 6, 2018

swissspidy added 6 commits August 6, 2018 16:56

Add test for exclusion glob patterns

9161db2

Add test for multiple merge files

b1cfb7b

Add test for file inclusion

b81cd40

Add test for string exceptions aka black list

9fcb6a9

Add test for generic project

dbc55b2

Add test for customized copyright notices

0f0af9e

swissspidy requested a review from a team August 6, 2018 16:02

Merge branch 'master' into 36-core

cc6f466

swissspidy mentioned this pull request Aug 6, 2018

Check for more mistakes in translatable strings #64

Merged

schlessera requested changes Aug 6, 2018

View reviewed changes

schlessera added this to the 0.1.0 milestone Aug 6, 2018

schlessera added the command:i18n-make-pot label Aug 6, 2018

swissspidy added 3 commits August 6, 2018 22:01

Merge branch 'master' into 36-core

7cd61ef

Incorporate feedback from code review

abf34a5

Adjust test for generic project to use _()

36dda78

schlessera approved these changes Aug 6, 2018

View reviewed changes

schlessera merged commit 066d581 into master Aug 6, 2018

schlessera deleted the 36-core branch August 6, 2018 21:30

schlessera added a commit that referenced this pull request Jan 6, 2022

Merge pull request #69 from wp-cli/36-core

7e36d97

WordPress Core support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WordPress Core support #69

WordPress Core support #69

swissspidy commented Aug 5, 2018 •

edited

Loading

swissspidy commented Aug 5, 2018

swissspidy commented Aug 6, 2018

swissspidy commented Aug 6, 2018

schlessera commented Aug 6, 2018

swissspidy commented Aug 6, 2018

schlessera commented Aug 6, 2018

schlessera commented Aug 6, 2018

swissspidy commented Aug 6, 2018

swissspidy commented Aug 6, 2018

swissspidy commented Aug 6, 2018

swissspidy commented Aug 6, 2018

schlessera commented Aug 6, 2018

schlessera Aug 6, 2018

swissspidy Aug 6, 2018

schlessera Aug 6, 2018

swissspidy Aug 6, 2018

schlessera Aug 6, 2018

schlessera Aug 6, 2018

schlessera Aug 6, 2018

swissspidy Aug 6, 2018

schlessera Aug 6, 2018

schlessera Aug 6, 2018

swissspidy Aug 6, 2018

schlessera Aug 6, 2018

swissspidy Aug 6, 2018

schlessera Aug 6, 2018

swissspidy Aug 6, 2018

swissspidy commented Aug 6, 2018

schlessera commented Aug 6, 2018

johnbillion commented Nov 9, 2018

		If left empty, defaults to the destination POT file.

		[--except=<files>]

WordPress Core support #69

WordPress Core support #69

Conversation

swissspidy commented Aug 5, 2018 • edited Loading

swissspidy commented Aug 5, 2018

swissspidy commented Aug 6, 2018

swissspidy commented Aug 6, 2018

schlessera commented Aug 6, 2018

swissspidy commented Aug 6, 2018

schlessera commented Aug 6, 2018

schlessera commented Aug 6, 2018

swissspidy commented Aug 6, 2018

swissspidy commented Aug 6, 2018

swissspidy commented Aug 6, 2018

swissspidy commented Aug 6, 2018

schlessera commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swissspidy commented Aug 6, 2018

schlessera commented Aug 6, 2018

johnbillion commented Nov 9, 2018

swissspidy commented Aug 5, 2018 •

edited

Loading