False-positive/dont apply translators comments multiple times

[kkmuffme]

Problems

1. Given this string:

printf( /* translators: %s: link */ esc_html__( 'Track your parcel with %s.', 'mytext' ), '<a ....>' . esc_html__( 'USPS' ) . '</a>' );

It will apply the translators comment not only to the first string, but also to the second string (USPS)

======

2)a) Additionally, if multiple strings are on the same line, it will apply the translators comment to ALL strings on this line, instead for 1 string only.

printf( /* translators: %s: test */ esc_html__( 'Hi %s', 'mytext' ), esc_html( $name ) ); esc_html( 'hello', 'my-text' );

b) and it even happens when they are not on the same line:

printf( /* translators: hello */ __( 'world', 'mytext' ) );
_e( 'Why here too?', 'mytext' );

See in the .pot:

#. translators: hello
#: myfile.php:1
msgid "world"
msgstr ""

#. translators: hello
#: myfile.php:2
msgid "Why here too?"
msgstr ""

======

Solution
do not apply 1 translators comment to more than 1 translateable string

Since it's normal to have multiple strings following each other, this is a severe bug that should be tackled asap.
If any testing (or anything else, besides creating a fix, bc I have 0 plan of it) is required, I am happy to help

Maybe related:
#96
#4 (this may have added the bug in the first place)

[swissspidy]

Heya, thanks for your report. I think this might be a duplicate of #96, at least in some parts. There's definitely some room for improvement here.

However, I also want to point out that your examples are not an ideal way to add translator comments IMHO. Simply because they make it not clear enough for translators and users which function call they refer to. Plus, it's more difficult to translate it that way. Not even mentioning readability of the code in general. That's why in WordPress core we use multi-line function calls for these cases.

For example, the first piece of code I'd write more like this:

printf(
  /* translators: %s: link */
  esc_html__( 'Track your parcel with %s.', 'mytext' ),
  '<a ....>' . esc_html__( 'USPS' ) . '</a>'
);

// Or even better:
printf(
  /* translators: %s: link */
  __( 'Track your parcel with <a href="%s">USPS</a>.', 'mytext' ),
  'https://...'
);

[kkmuffme]

Yep I know it's partially duplicate, thats why I linked #96

Thanks for your feedback, the reason it is all stuck into 1 line in my examples is that it's much easier to do a "Replace in all files" ehn the string is in 1 line.
Since we handle loads of projects that have partly identical strings in their templates, doing it like this is much faster than having them multi-line.

Additionally, for options arrays, e.g.
'description' => ...
putting it on one line makes it more readable

Also here:
__( 'Track your parcel with <a href="%s">USPS</a>.', 'mytext' ),

This is how we try to do it, unfortunately the a href tag in this case has loads of long data attributes for analytics and whatnot that shouldn't get translated.
The only way we could do it it would be like:
__( 'Track your parcel with <a %s>USPS</a>.', 'mytext' ),

But since this grew organically it would mean retranslating 100s of strings in 10+ languages which isn't an option atm.

[swissspidy]

Just encountered this myself now, but in a JS file. JS files are often minified, so the issue might occur more often there.

To fix this, there could be a simple cache of comment nodes (key value store) where we store nodes that were already used/applied. So when extracting the next string and its comment, we we can simply look up that cache.

This would need to happen in these two places:

i18n-command/src/JsFunctionsScanner.php

Lines 331 to 355 in 606bbab

	/**
	* Returns wether or not a comment precedes a node.
	* The comment must be before the node and on the same line or the one before.
	*
	* @param Node\Comment $comment The comment.
	* @param Node\Node $node The node.
	*
	* @return bool Whether or not the comment precedes the node.
	*/
	private function commentPrecedesNode( Node\Comment $comment, Node\Node $node ) {
	// Comments should be on the same or an earlier line than the translation.
	if ( $node->getLocation()->getStart()->getLine() - $comment->getLocation()->getEnd()->getLine() > 1 ) {
	return false;
	}
	// Comments on the same line should be before the translation.
	if (
	$node->getLocation()->getStart()->getLine() === $comment->getLocation()->getEnd()->getLine() &&
	$node->getLocation()->getStart()->getColumn() < $comment->getLocation()->getStart()->getColumn()
	) {
	return false;
	}
	return true;
	}

i18n-command/src/PhpFunctionsScanner.php

Lines 73 to 80 in 606bbab

	$translation = $translations->insert( $context, $original, $plural );
	$translation = $translation->addReference( $file, $line );
	if ( isset( $function[3] ) ) {
	foreach ( $function[3] as $extracted_comment ) {
	$translation = $translation->addExtractedComment( $extracted_comment );
	}
	}

[swissspidy]

#261 fixes this for JS files. For PHP files unfortunately this has to be resolved in the upstream gettext library (https://github.com/php-gettext/Gettext/blob/58bc0f7f37e78efb0f9758f93d4a0f669f0f84a1/src/Utils/PhpFunctionsScanner.php#L58-L165) if not already fixed in the latest release.

[kkmuffme]

Thanks.

Could you confirm whether this has been fixed in the latest release for gettext? As otherwise I think we should open an issue there

[swissspidy]

Just did a quick test by adding your cases from above as test cases in https://github.com/php-gettext/PHP-Scanner (from gettext v5) and was not able to reproduce the issue. So it looks like updating the gettext library should fix this 🎉

The only problem is we cannot update to gettext v5 due to PHP version constraints (see #217). Not sure if v4 still receives support, but in v4 this issue is still present.

[kkmuffme]

Could we not just use gettext v5 conditionally? (if php >= 7.2)

[swissspidy]

I don't think that's possible nor feasible as it would essentially mean maintaining two code bases.

[wojsmol]

Additionally wp-cli-bundle enforces php 5.6 https://github.com/wp-cli/wp-cli-bundle/blob/398a1d8f618bfa5c4b1eb8300004153ff3efa267/composer.json#L47-L50

[kkmuffme]

Would it work if I globally (I know it's a no-no, but still) install gettext v5 on my machine? or would it still fall back to the old gettext in this case?

[swissspidy]

Using a globally installed dependency like that is not possible.

Anyway, I just asked the developer and he accepts patches for gettext v4 if they fix bugs. So if we can fix the bug in v4 that would be the best (and only realistic) route going forward.

[schlessera]

@kkmuffme I fully realize how frustrating this is, but it is unfortunately part of the deal if WordPress is your weapon of choice. We are having more and more problems with the tooling all around the WP ecosystem, and we're trying our best to keep everything working no matter what, but it is what it is at this point.

[schlessera]

The only other option we have is adding patches via a Composer plugin, but I'd prefer to avoid that, as it has led to more trouble than it solved in the past in some circumstances.

[kkmuffme]

@schlessera yes I think starting to patch things is not really worth the effort, since it's been more than 2 years already since this issue surfaced.

offtopic: the backwards compatibility is really hurting the future of WP currently. e.g. wp-cli still generates notices on PHP 8.0 (despite 8.1 being released), but there is support for completely outdated PHP 5.6.x

[schlessera]

The remaining notices on PHP 8 are not coming from WP-CLI, but rather from WordPress Core and - more importantly - from plugins and themes.

PHP is moving so fast that it makes it almost impossible to keep legacy projects in the loop, which has been a growing negative sentiment amongst all the package maintainers that are trying (and failing) to keep up.

WordPress Core and its ecosystem is not even close to properly supporting PHP 8. All it has going in terms of benefits right now is that it doesn't just bump its PHP version requirement like most other projects do. That single fact is probably the biggest part in the success of WordPress. I dislike the engineering implications as much as you do, but you can't have WordPress without it.

[swissspidy]

Anyway, I just asked the developer and he accepts patches for gettext v4 if they fix bugs. So if we can fix the bug in v4 that would be the best (and only realistic) route going forward.

I've now submitted an upstream PR to fix this in Gettext v4: php-gettext/Gettext#271

[swissspidy]

It was just merged 🎉

Once there's a new release of the gettext library we can finally close this ticket

[swissspidy]

https://github.com/php-gettext/Gettext/releases/tag/v4.8.5

[kkmuffme]

Thanks a lot!

Just to confirm: what do I need to do now to get this working version? Update wp-cli nightly?

[swissspidy]

Since there was no new release yet of this package and the wp-cli nightlies are currently outdated, try wp package install wp-cli/i18n-command instead for now to get the current development version.

If that doesn't work, we might need to tag a new release first.