Skip to content
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

PHP i18n functions prefixed with a slash are ignored #344

Closed
2 tasks done
dgwyer opened this issue Nov 16, 2022 · 11 comments
Closed
2 tasks done

PHP i18n functions prefixed with a slash are ignored #344

dgwyer opened this issue Nov 16, 2022 · 11 comments

Comments

@dgwyer
Copy link

dgwyer commented Nov 16, 2022

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
Copy link
Member

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
Copy link
Author

dgwyer commented Nov 22, 2022

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.

php-gettext/Gettext#284 (comment)

@remcotolsma
Copy link

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
Copy link

remcotolsma commented Dec 7, 2022

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:

'__' => 'text_domain',
'esc_attr__' => 'text_domain',
'esc_html__' => 'text_domain',
'esc_xml__' => 'text_domain',
'_e' => 'text_domain',
'esc_attr_e' => 'text_domain',
'esc_html_e' => 'text_domain',
'esc_xml_e' => 'text_domain',
'_x' => 'text_context_domain',
'_ex' => 'text_context_domain',
'esc_attr_x' => 'text_context_domain',
'esc_html_x' => 'text_context_domain',
'esc_xml_x' => 'text_context_domain',
'_n' => 'single_plural_number_domain',
'_nx' => 'single_plural_number_context_domain',
'_n_noop' => 'single_plural_domain',
'_nx_noop' => 'single_plural_context_domain',

@swissspidy
Copy link
Member

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
Copy link

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
Copy link
Member

swissspidy commented Dec 7, 2022

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

@remcotolsma
Copy link

remcotolsma commented Dec 7, 2022

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
Copy link
Member

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
Copy link

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

@swissspidy
Copy link
Member

swissspidy commented Dec 9, 2022

Working on adding a test here: #344

Updating the dependency here:

danielbachhuber pushed a commit to wp-cli/wp-cli-bundle that referenced this issue Dec 9, 2022
ferdysopian added a commit to ferdysopian/i18n-command that referenced this issue Dec 20, 2022
* main:
  Escape backslashes
  Add test for wp-cli#344
  PHP 8.2: fix "Using ${var} in strings is deprecated, use {$var} instead"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants