Make WordPress Core

Opened 4 years ago

Closed 5 months ago

Last modified 4 months ago

#52696 closed enhancement (fixed)

Add a way to determine whether a translation exists

Reported by: drzraf's profile drzraf Owned by: swissspidy's profile swissspidy
Milestone: 6.7 Priority: normal
Severity: minor Version:
Component: I18N Keywords: good-first-bug has-patch has-unit-tests commit has-dev-note
Focuses: Cc:

Description

Currently __() wraps pomo/translations.php::translate_entry()̀ in such a way:

<?php
                function translate( $singular, $context = null ) {
                        $entry      = new Translation_Entry(
                                array(
                                        'singular' => $singular,
                                        'context'  => $context,
                                )
                        );
                        $translated = $this->translate_entry( $entry );
                        return ( $translated && ! empty( $translated->translations ) ) ? $translated->translations[0] : $singular;
                }

If no translation exist, the original is returned which is the more frequently desirable behavior.

But in some cases, the user may want to know whether the string obtained from __() is actually the translation or the original.

Use-case: I'm inserting programmatically taxonomy terms translated using a 3rd-party service. I'm iterating over [__("t1"), __("t2"), ...] and I would like to only insert those actually translated, ignoring the others.

Currently, I've no way to discriminate between the original and the translation (which may or may not be identical btw).

I'd like a function providing me this ability like. One such example
__i18n_exists($term, $context = null);

Attachments (1)

52696.diff (2.1 KB) - added by tomhine 9 months ago.
SVN diff for the addition of has_translation functionality

Download all attachments as: .zip

Change History (38)

#1 @swissspidy
3 years ago

  • Milestone changed from Awaiting Review to Future Release

This would make it consistent with the @wordpress/i18n JS package which has a hasTranslation function for this purpose as well.

#2 @swissspidy
21 months ago

  • Keywords needs-patch good-first-bug added
  • Version 5.6.2 deleted

#3 @samuelsilvapt
21 months ago

Hi! I'm exploring an approach like:

is_translated( $string, $domain );
is_translated_n( $string, $domain );

#4 @ckanitz
21 months ago

Hi!

I've startet working on this on the WCEU contributor day. My first approach of the requested feature looks like this:

<?php
function __i18n_exists( $text, $domain = 'default' ) {
        $translations = get_translations_for_domain( $domain );
        $translation  = $translations->translate( $text );

        if ( empty( $translations->entries ) ) {
                return null;
        }

        return $translation;
}

If I test it against de_DE translations "Post" translates to "Beitrag" but en_EN will just return null.

Let me know if that's the kind of behaviour you're looking for.

#5 @samuelsilvapt
21 months ago

@ckanitz I'm here too! What's your name? I'll try to find you :D

About your approach, I think we should return always a bool, not a string.
So, what do you think about this:

<?php
return empty( $translations->entries );

This ticket was mentioned in PR #4581 on WordPress/wordpress-develop by ckanitz.


21 months ago
#6

  • Keywords has-patch added; needs-patch removed

Worked together with @samuelsilvapt (wp.org profile: https://profiles.wordpress.org/samuelsilvapt/) on this.

We updated the __ and translate() function within /wp-includes/i18n.php as well as Translations::translate() function to support a new optional parameter $return_singular.

The new parameter defaults to true but if set to false, instead of the inputed $text as a fallback if no translation exists for that string null will returned instead.

We also introduced a new function has_translation( $text, $domain ) for explicit checks (like hasTranslation is already part of the @wordpress/i18n package).

Trac ticket: https://core.trac.wordpress.org/ticket/52696

This ticket was mentioned in Slack in #core by samuelsilvapt. View the logs.


21 months ago

@swissspidy commented on PR #4581:


21 months ago
#8

Great progress so far 👍

I would just recommend that instead of adding a new parameter to the translate() method, there should be a new has_translation() method. Also no need for changes to the __() function.

Something like this:

function has_translation( $text, $domain = 'default' ) {
        $translations = get_translations_for_domain( $domain );
        return $translations->has_translation( $text );
}

That makes everything much easier to understand and more maintainable.

#9 @ckanitz
21 months ago

Thanks for the feedback! I got the idea of your recommendation and agree, that has_translation() should be part of the Translate() (and NOOP_Translate()) class in translations.php and a wrapper in l10n.php.

But that's just for the helper functions, which is nice to have, but wouldn't be quiet "elegant" if you'd have to loop over a bunch of strings instead of just calling the known translation functions with an extra parameter tweaking the returned value.

We've started with an own function but since it was just a copy of __ we dropped it and thought about a more DRY approach with a new optional nonbreaking parameter.

If it's a real "no-go" to update the existing functions I'd propose for a new function after all but could need some help to find a better name instead of __i18n_exists.

#10 @swissspidy
21 months ago

  • Keywords needs-refresh added

Repurposing __() in such a way is a non-goal, so I would really just add a separate translation_exists() or has_translation function and a separate method on the translations class as described in my previous comment.

#11 @swissspidy
12 months ago

  • Keywords needs-patch added; has-patch needs-refresh removed

#12 @swissspidy
12 months ago

  • Summary changed from __() avoiding default language fallback to Add a way to determine whether a translation exists

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


11 months ago

#14 @mchirag2002
10 months ago

I would like to work on this ticket

#15 @swissspidy
10 months ago

Any contributor is welcome to work on a patch.

Note that a lot has changed in regards to i18n as we added a new i18n library in 6.5. So this would require working on WP_Translation_Controller now.

@tomhine
9 months ago

SVN diff for the addition of has_translation functionality

#16 @tomhine
9 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

Added a new has_translation public method to the WP_Translation_Controller which calls the protected locate_translation method to determine if a translation is available for the given string (with optional textdomain and locale).

Also, exposed this in l10n.php.

Just a side note that this is my first attempt at contributing here so apologises for any mistakes on my end.

Last edited 9 months ago by tomhine (previous) (diff)

#17 @swissspidy
9 months ago

  • Keywords needs-unit-tests added

Thanks, that's a great starting point!

To make collaboration and code review easier, I recommend creating a PR on GitHub. It will also make it easier to address PHPCS issues and work on unit tests.

Functionality-wise, we don't need the null === $locale check in the has_translation() function as WP_Translation_Controller::has_translation() already does that. So it's redundant.

This ticket was mentioned in PR #6649 on WordPress/wordpress-develop by @tomhine.


9 months ago
#18

Added a new has_translation public method to the WP_Translation_Controller which calls the protected locate_translation method to determine if a translation is available for the given string (with optional textdomain and locale).
Also, exposed this in l10n.php.

Trac ticket: https://core.trac.wordpress.org/ticket/52696#comment:17

#19 @mchirag2002
9 months ago

@swissspidy
The translate function present in the WP_Translation_Controller is already utilizing this locate_translation() protected function but in the l10n.php file we are still using the NOOP Translate instance based translate function.
Wouldn't it be more efficient if we switch to using the WP_Translation_Controller's translate function like we are utilising the set_locale() function from the controller?
That way we would not be required to make a separate call to the has_translation() first and then call the translating functions.

Last edited 9 months ago by mchirag2002 (previous) (diff)

#20 @swissspidy
9 months ago

Not sure I follow. You mean you want to utilize WP_Translation_Controller's in __() & co? That seems like an entirely different change (with potential backward compatibility implications). And not sure how that would address this ticket.

But I might be misunderstanding your request. In that case, feel free to open a PR with a suggested code change or so.

FWIW no one is required to use both functions. I consider has_translation() an edge case that rarely ever someone truly needs. Most of the time you should just translate.

This ticket was mentioned in PR #6658 on WordPress/wordpress-develop by @mchirag2002.


9 months ago
#21

The added function has_translate() tackles the edge case where the user might need to check if the translation exists or not before actually translating the string.
The function returns the translation of the string if it exists and returns false if in case the translation does not exist.
This should potentially save the multiple calls for the functions like once to check whether translation exists or not and then to actually translate.

Trac ticket: https://core.trac.wordpress.org/ticket/52696

#22 @mchirag2002
9 months ago

@swissspidy
Please take a look at this implementation made in the PR. This way in the edge case that the user might require to check whether the translation exists or not, they wouldn't have to make a separate function call to actually translate the ones which have translations available. The functionality was already present in the WP_Translations_Controller.

@swissspidy commented on PR #6658:


9 months ago
#23

This does not work because you don't know if the returned string is the original or just a translation that happens to be identical to the original string.

That's why #6649 works better.

@mchirag2002 commented on PR #6658:


9 months ago
#24

@swissspidy Actually in this case the original string would never be returned. If there is no translation present, it would return a boolean false and if there exists a string which has a translation which is identical, only in such case the translated string shall be returned.
Internally the translate function also utilises the same locate_translate() function but this reduces the number of function calls you have to make to get the translation of all the strings.

@swissspidy commented on PR #6658:


9 months ago
#25

if there exists a string which has a translation which is identical, only in such case the translated string shall be returned.

And that is exactly the problem I am describing. This is not desired here.

This ticket was mentioned in PR #6805 on WordPress/wordpress-develop by @louiswol94.


8 months ago
#26

  • Keywords has-unit-tests added; needs-unit-tests removed

#27 follow-up: @louiswol94
8 months ago

Hi everyone,

I am sitting here at Contributor Day at WCEU, and by the tags I saw this was a good first ticket, and that only Unit tests were still needed.

I gathered that #6649 was the accepted solution, so I copied that code, and added some unit tests here: https://github.com/WordPress/wordpress-develop/pull/6805

The original author is welcome to use my tests as inspiration, or we can merge my PR, I am not sure what is the best way forward.

Kind regards

#28 in reply to: ↑ 27 @goldhat
5 months ago

Replying to louiswol94:

Hi everyone,

I am sitting here at Contributor Day at WCEU, and by the tags I saw this was a good first ticket, and that only Unit tests were still needed.

I gathered that #6649 was the accepted solution, so I copied that code, and added some unit tests here: https://github.com/WordPress/wordpress-develop/pull/6805

The original author is welcome to use my tests as inspiration, or we can merge my PR, I am not sure what is the best way forward.

Kind regards

Nice work. I'm just looking at this to see if anything is left to be done. It looks like the solution is agreed on and you have added tests.

A couple of the links included in comments here point to 6649 on trac and I think they were meant to link to the PR on GitHub: https://github.com/WordPress/wordpress-develop/pull/6649. Use full URL's to GitHub because Trac auto-links tickets if the number happens to match.

#29 @louiswol94
5 months ago

@goldhat Thanks for checking in! Should I do anything here on my side?

@swissspidy commented on PR #6649:


5 months ago
#30

Closing in favor of #6805 which builds upon this and adds tests

@swissspidy commented on PR #6658:


5 months ago
#31

Closing in favor of #6805 which is closer and adds tests

#32 @swissspidy
5 months ago

  • Milestone changed from Future Release to 6.7
  • Owner set to swissspidy
  • Status changed from new to reviewing

#33 @swissspidy
5 months ago

  • Keywords commit needs-dev-note added; needs-testing removed

#34 @swissspidy
5 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 59029:

I18N: Add a new way to determine whether a translation is available.

A new has_translation() function can be used to determine whether a translation exists for a given string.

Props louiswol94, swissspidy, drzraf, ckanitz, tomhine, mchirag2002, samuelsilvapt.
Fixes #52696.

@louiswol94 commented on PR #6805:


5 months ago
#36

Thanks @swissspidy

#37 @swissspidy
4 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.