search

wp-cli / i18n-command

Provides internationalization tools for WordPress projects.
MIT License
96 stars 52 forks source link

PHP i18n functions prefixed with a slash are ignored #344

Closed dgwyer closed 1 year ago

dgwyer commented 1 year ago

Bug Report

Running the latest version of wp-cli (2.7.1) i18n functions prefixed with a slash are ignored. I need to prefix with a slash as I'm using PHP namespaces in my project.

Minimal reproducible example:

<?php
// wp i18n make-pot . --domain=bar
\esc_html__('foo', 'bar'); // doesnt work
esc_html__('baz', 'bar'); // works

The output from the snippet above is:

msgid ""
msgstr ""
"Project-Id-Version: \n"
"Report-Msgid-Bugs-To: \n"
"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
"Language-Team: LANGUAGE <LL@li.org>\n"
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
"Content-Transfer-Encoding: 8bit\n"
"POT-Creation-Date: 2022-11-16T15:11:24+00:00\n"
"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
"X-Generator: WP-CLI 2.7.1\n"
"X-Domain: bar\n"

#: test-plugin.php:4
msgid "baz"
msgstr ""
swissspidy commented 1 year ago

Well that is surprising!

Looks like the PHP functions scanner we're using doesn't properly extract such functions call, so unfortunately this will need to get fixed upstream.

dgwyer commented 1 year ago

Just adding a note here that this isn't likely to be fixed in the Gettext repo any time soon due to time constraints; incase anyone from the wp-cli team is able to pick this up.

https://github.com/php-gettext/Gettext/issues/284#issuecomment-1321112613

remcotolsma commented 1 year ago

It might be good to know that this seems to work well on PHP 7.4, but it goes wrong in PHP 8.

A workaround is to run wp i18n make-pot on PHP 7.4:

brew link --overwrite php@7.4
remcotolsma commented 1 year ago

Maybe it's related to a token_get_all() change in PHP 8: https://3v4l.org/48MdV

In PHP 7.4 the function call \__ is split in two tokens \ and __ in PHP 8.0 it's just one token \__.

In PHP 8.0 there is a new T_NAME_FULLY_QUALIFIED token: https://www.php.net/manual/en/tokens.php

It could be fixed by adding T_NAME_FULLY_QUALIFIED to the following line: https://github.com/php-gettext/Gettext/blob/207719c6eae36f5ac7c2c9542f6a4d4b76307e5a/src/Utils/PhpFunctionsScanner.php#L101

And by adding fully qualified function names (\__, \_e, etc.) to this list:

https://github.com/wp-cli/i18n-command/blob/c1beab89fea0df56e23e460a5504ce30940a86b7/src/PhpCodeExtractor.php#L17-L33

swissspidy commented 1 year ago

Interesting, yeah that might be it. In any case, it needs a volunteer to submit a PR to the Gettext library to help fix this properly :)

remcotolsma commented 1 year ago

The https://github.com/php-gettext/Gettext library is already moved on the version 5 and the new https://github.com/php-gettext/PHP-Scanner library uses https://github.com/nikic/php-parser. It is quite possible that https://github.com/nikic/php-parser has better support for fully qualified names (FQN) and the problem is therefore no longer an issue in the latest Gettext library?

swissspidy commented 1 year ago

Yes it's quite possible, but we cannot use the newer version yet because of PHP version requirements.

remcotolsma commented 1 year ago

In any case, it needs a volunteer to submit a PR to the Gettext library to help fix this properly :)

I have come up with a fix and opened a PR:

I don't have much experience with this, but I did my best to contribute.

swissspidy commented 1 year ago

Awesome, thanks a lot! And it has been merged, too!

Once a new release of Gettext v4 has been released, we can update the dependency here & update our tests too. Then we should be all set.

remcotolsma commented 1 year ago

ℹī¸ Version 4.8.8 of gettext/gettext was released yesterday: https://github.com/php-gettext/Gettext/releases/tag/v4.8.8 🎉.

swissspidy commented 1 year ago

Working on adding a test here: #344

Updating the dependency here: