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

New feature: Translation message placeholder support #121

Draft
wants to merge 18 commits into
base: 2.27.x
Choose a base branch
from

Conversation

TotalWipeOut
Copy link

@TotalWipeOut TotalWipeOut commented May 16, 2024

Q A
Documentation not yet
Bugfix no
BC Break no
New Feature yes
RFC yes
QA no

Description

WORK IN PROGRESS

Upon needing message placeholders and seeing the 4 year old issue #7 about needing it, I decided to take a stab at adding message placeholder support into laminas-i18n, as I see it as key missing feature.

(I decided to use the term 'placeholders' instead of 'parameters' so that it is less ambiguous)

As I really didn't want a BC break, I have hijacked the $textDomain parameter of the translate / translatePlural method to allow passing in placeholders where it seemed most logical going forward. There is a way to be able to provide both textDomain and placeholders if that's needed.

Goals

  • Allow placeholders
  • Support multiple kinds of placeholder formats
  • No BC breaks
  • 100% test coverage

What I've done so far

  • Added a placeholder plugin manager
  • Added initial set of placeholder compilers
  • Modified Translator\TranslatorServiceFactory to inject a placeholder from config
  • Modified Translator\Translator class to add new methods that pass messages through a placeholder compiler before returning
  • New view helpers to utilise new Translator methods
  • Seems to work at a basic level
  • Existing tests are all passing

TODO

  • have not yet written any tests for the new classes
  • The placeholders need work

Before i go any further, I am seeking advice as to if this is the correct approach for adding this without a bc break

The code is not perfect, so any guidance on naming/architecture/design would be amazing. Once i have some feedback, i will make any required changes and get on with the tests

And, a thank you to everyone in the Laminas community for being so kind and generous with your time ❤️

@froschdesign
Copy link
Member

froschdesign commented May 17, 2024

@TotalWipeOut
First: Thank you very much for your help!


Some comments:

I decided to use the term 'placeholders' instead of 'parameters' so that it is less ambiguous

A placeholder is in the message: "Hello {name}!" and the process of converting the message with a placeholder can be called formatting or replacement. To format or replace the placeholders, values/parameters are required.
Therefore, the naming with name "placeholders" in your code is incorrect.

As I really didn't want a BC break, I have hijacked the $textDomain parameter of the translate / translatePlural method to allow passing in placeholders where it seemed most logical going forward.

Hijacking is not an option because:

  • the interface defines the parameter with the type string
    * @param string $textDomain
  • with your solution, a rewrite of the arguments is required for the next major version
  • and any hidden or magical use is not desired and must therefore be avoided
  • Support multiple kinds of placeholder formats

My first thought was, why allow multiple variants when one official is enough, but if we see this component in use with different frameworks or CMS, this would be a benefit.
On the other hand, with the ICU MessageFormat the translation with plural can be done with one method, and we can kill translatePlural.

  • No new view helpers

Why not? We can update the view helper configuration to use the new view helpers with parameter support:

/**
* Return laminas-view helper configuration.
*
* Obsoletes View\HelperConfig.
*
* @return ServiceManagerConfigurationType
*/
public function getViewHelperConfig()

And the new view helper(s) can be used like this:

<?= $this->translate($message, params: ['name' => 'World']) ?>

The old view helpers must be retained for reasons of backward compatibility.

@TotalWipeOut
Copy link
Author

@froschdesign
Thanks for explaining placeholders vs parameters. I will adjust the code naming.

The reason for multiple placeholder formats is that while I agree ICU message format is the place to be, i wanted the ability to allow this feature to integrate with existing apps with existing translation files that might do it in the various other ways via custom solutions. The app I work on uses printf format for this task currently

Should I add a new method in Translator/Translator called translateWithParams or similar instead?

Regarding view helpers, from your example, can you show me how this will work with a new view helper, but called the same in the view and it not being a BC break? I don't fully understand

Thanks again for your detailed response ❤️

@froschdesign
Copy link
Member

froschdesign commented May 17, 2024

Regarding view helpers, from your example, can you show me how this will work with a new view helper, but called the same in the view and it not being a BC break? I don't fully understand

This is a simple mapping:

$renderer = new Laminas\View\Renderer\PhpRenderer();
$renderer->getHelperPluginManager()->configure(
    (new Laminas\I18n\ConfigProvider())->getViewHelperConfig()
);
$renderer->getHelperPluginManager()->setFactory(
    NewTranslate::class,
    Laminas\ServiceManager\Factory\InvokableFactory::class
);
$renderer->getHelperPluginManager()->setAlias(
    'translate',
    NewTranslate::class
);

echo $renderer->translate('example', params: ['foo' => 'bar']);

This overwrites the existing view helper, as is done if you want to use your own.

@TotalWipeOut
Copy link
Author

@froschdesign So i understand correctly with this approach, this functionality requires additional config in the app, add wont work out of the box?
If so, for something accessible out of the box, would adding a new 4th param to the Translate view helper be considered a bc break? As a view helper called translateWithParams feels too long for something that is used everywhere
I see other frameworks use t or trans to keep it short, could we use a name like that?
Thanks again for your time.

@froschdesign
Copy link
Member

froschdesign commented May 17, 2024

So i understand correctly with this approach, this functionality requires additional config in the app, add wont work out of the box?

This component already provides a configuration:

/**
* Return laminas-view helper configuration.
*
* Obsoletes View\HelperConfig.
*
* @return ServiceManagerConfigurationType
*/
public function getViewHelperConfig()
{
return [
'aliases' => [
'countryCodeDataList' => View\Helper\CountryCodeDataList::class,
'currencyformat' => View\Helper\CurrencyFormat::class,
'currencyFormat' => View\Helper\CurrencyFormat::class,
'CurrencyFormat' => View\Helper\CurrencyFormat::class,
'dateformat' => View\Helper\DateFormat::class,
'dateFormat' => View\Helper\DateFormat::class,
'DateFormat' => View\Helper\DateFormat::class,
'numberformat' => View\Helper\NumberFormat::class,
'numberFormat' => View\Helper\NumberFormat::class,
'NumberFormat' => View\Helper\NumberFormat::class,
'plural' => View\Helper\Plural::class,
'Plural' => View\Helper\Plural::class,
'translate' => View\Helper\Translate::class,
'Translate' => View\Helper\Translate::class,
'translateplural' => View\Helper\TranslatePlural::class,
'translatePlural' => View\Helper\TranslatePlural::class,
'TranslatePlural' => View\Helper\TranslatePlural::class,
// Legacy Zend Framework aliases
'Zend\I18n\View\Helper\CurrencyFormat' => View\Helper\CurrencyFormat::class,
'Zend\I18n\View\Helper\DateFormat' => View\Helper\DateFormat::class,
'Zend\I18n\View\Helper\NumberFormat' => View\Helper\NumberFormat::class,
'Zend\I18n\View\Helper\Plural' => View\Helper\Plural::class,
'Zend\I18n\View\Helper\Translate' => View\Helper\Translate::class,
'Zend\I18n\View\Helper\TranslatePlural' => View\Helper\TranslatePlural::class,
],
'factories' => [
View\Helper\CountryCodeDataList::class => View\Helper\Container\CountryCodeDataListFactory::class,
View\Helper\CurrencyFormat::class => InvokableFactory::class,
View\Helper\DateFormat::class => InvokableFactory::class,
View\Helper\NumberFormat::class => InvokableFactory::class,
View\Helper\Plural::class => InvokableFactory::class,
View\Helper\Translate::class => InvokableFactory::class,
View\Helper\TranslatePlural::class => InvokableFactory::class,
],
];
}

This must be extended so that the new view helper(s) can be used.

If so, for something accessible out of the box, would adding a new 4th param to the Translate view helper be considered a bc break?

No, because a new view helper must be created and the old one remains as it is. (The view helpers are not marked as final, so we cannot change them.)

As a view helper called translateWithParams feels too long for something that is used everywhere
I see other frameworks use t or trans to keep it short, could we use a name like that?

No, nothing must and will be changed in this case. Via the configuration we will map translate to the new view helper which supports parameters. It is not necessary to change anything in the view scripts, unless you want to use the parameters, in which case the method call must be extended:

// Before:
$this->translate($message);

// After:
$this->translate($message, params: ['name' => 'World']);

This works out of the box.


Please look at the renderer of laminas-view:

@TotalWipeOut
Copy link
Author

@froschdesign Thanks - I have tried to address your feedback :)

@TotalWipeOut TotalWipeOut changed the title New feature: Translation placeholder support New feature: Translation message placeholder support May 18, 2024
@TotalWipeOut
Copy link
Author

@froschdesign Let me know if this is something your happy to be included in this library, and i will get the tests written. Thanks
CC @weierophinney for visibility, as you raised the original issue.

@froschdesign
Copy link
Member

froschdesign commented May 22, 2024

@TotalWipeOut
Sorry for the late response! I was busy with this task: #90

Let me know if this is something your happy to be included in this library…

Short: The feature is definitely necessary, but with a modified implementation.

Minor Release

If we add this feature in a minor release then without any changes on the Translator class.

Example

namespace Laminas\I18n\Translator\Formatter;

use Laminas\I18n\Translator\Translator;
use Laminas\I18n\Translator\TranslatorInterface;

final class TranslatorFormatterDecorator implements TranslatorInterface
{
    public function __construct(
        private readonly Translator $translator,
        private readonly FormatterInterface $formatter
    ) {
    }

    public function translate(
        $message,
        $textDomain = 'default',
        $locale = null,
        iterable $parameters = []
    ): string {
        // …
    }

    public function translatePlural(
        $singular,
        $plural,
        $number,
        $textDomain = 'default',
        $locale = null,
        iterable $parameters = []
    ): string {
        // …
    }
}

Benefits

  • no need to modify or extend the Translator class
  • no need introduce an interface which is obsolete with the next major version
  • the decorator:
    • can be added via delegator to the new view helpers
    • can easily removed with the next major version when the interface for the translator is modified
    • the user does not have to manually add or remove them

More Informations

Major Release

Here we can change the interface for the translator and rework the entire Translator class.


So if you can wait, the next major version is an option.

@froschdesign
Copy link
Member

@TotalWipeOut

CC @weierophinney for visibility, as you raised the original issue.

Not entirely true: "Originally posted by @thexpand at zendframework/zend-i18n#104" (See: #7)


use function str_replace;

class HandlebarPlaceholder implements PlaceholderInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming is still wrong because it is an formatter which formats a message. A placeholder is in the message: "Hello {name}!" – here {name}.

Copy link
Author

@TotalWipeOut TotalWipeOut May 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@froschdesign so then from this, shall i rename things like this:

  • namespace to be Laminas\I18n\Translator\Formatter
  • PlaceholderPluginManager to FormatterPluginManager, same with factory
  • PlaceholderInterface to FormatterInterface
  • and all class suffixes to *Formatter

or, if you have something else in mind, please advise

@TotalWipeOut
Copy link
Author

@TotalWipeOut

CC @weierophinney for visibility, as you raised the original issue.

Not entirely true: "Originally posted by @thexpand at zendframework/zend-i18n#104" (See: #7)

Oh yes, thanks for spotting that.

Signed-off-by: Kevin Hamilton <[email protected]>
Comment on lines +200 to +201
View\Helper\TranslateWithParams::class => InvokableFactory::class,
View\Helper\TranslatePluralWithParams::class => InvokableFactory::class,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will provide the delegator to add the decorator for the new view helpers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, I was going to ask regarding that. That would be great.
Thanks for all your time on this!

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

Successfully merging this pull request may close these issues.

2 participants