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

WordPress Core support #69

Merged
merged 20 commits into from Aug 6, 2018

Conversation

2 participants
@swissspidy
Collaborator

swissspidy commented Aug 5, 2018

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 #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.

  1. 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
  1. 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.

swissspidy added some commits Aug 5, 2018

@swissspidy

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Aug 5, 2018

Collaborator

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 :-)

Collaborator

swissspidy commented Aug 5, 2018

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

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Aug 6, 2018

Collaborator

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.

Collaborator

swissspidy commented Aug 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Aug 6, 2018

Collaborator

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:

  1. Generate frontend POT file
  2. Generate admin POT file
  3. Merge frontend with admin file and only keep the common strings
  4. 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.

Collaborator

swissspidy commented Aug 6, 2018

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:

  1. Generate frontend POT file
  2. Generate admin POT file
  3. Merge frontend with admin file and only keep the common strings
  4. 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

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 6, 2018

Member

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

Member

schlessera commented Aug 6, 2018

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

@swissspidy

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Aug 6, 2018

Collaborator

@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
Collaborator

swissspidy commented Aug 6, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 6, 2018

Member

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.

Member

schlessera commented Aug 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 6, 2018

Member

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

Member

schlessera commented Aug 6, 2018

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

@swissspidy

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Aug 6, 2018

Collaborator

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).

Collaborator

swissspidy commented Aug 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Aug 6, 2018

Collaborator

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.

Collaborator

swissspidy commented Aug 6, 2018

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

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Aug 6, 2018

Collaborator

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 :-)

Collaborator

swissspidy commented Aug 6, 2018

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 swissspidy changed the title from [WIP] Experiment with WordPress core support to WordPress Core support Aug 6, 2018

swissspidy added some commits Aug 6, 2018

@swissspidy

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Aug 6, 2018

Collaborator

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.

Collaborator

swissspidy commented Aug 6, 2018

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.

@swissspidy swissspidy requested a review from wp-cli/committers Aug 6, 2018

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 6, 2018

Member

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

Member

schlessera commented Aug 6, 2018

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

Show outdated Hide outdated README.md
Show outdated Hide outdated README.md
Show outdated Hide outdated README.md
"""
<?php
__( 'Hello World' );

This comment has been minimized.

@schlessera

schlessera Aug 6, 2018

Member

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

@schlessera

schlessera Aug 6, 2018

Member

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

This comment has been minimized.

@swissspidy

swissspidy Aug 6, 2018

Collaborator

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

@swissspidy

swissspidy Aug 6, 2018

Collaborator

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

This comment has been minimized.

@schlessera

schlessera Aug 6, 2018

Member

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

@schlessera

schlessera Aug 6, 2018

Member

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

Show outdated Hide outdated src/MakePotCommand.php
@swissspidy

This comment has been minimized.

Show comment
Hide comment
@swissspidy

swissspidy Aug 6, 2018

Collaborator

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

Collaborator

swissspidy commented Aug 6, 2018

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

@schlessera schlessera added this to the 0.1.0 milestone Aug 6, 2018

swissspidy added some commits Aug 6, 2018

@schlessera schlessera merged commit 066d581 into master Aug 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@schlessera schlessera deleted the 36-core branch Aug 6, 2018

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 6, 2018

Member

🎉

Member

schlessera commented Aug 6, 2018

🎉

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