Skip to content

[Translation] Translatable objects #37670

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

Merged
merged 1 commit into from
Aug 28, 2020

Conversation

natewiebe13
Copy link
Contributor

@natewiebe13 natewiebe13 commented Jul 26, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets None
License MIT
Doc PR symfony/symfony-docs#... - Will be added if this gets approval

Scenario
When writing business logic, this code typically lives within services. With complex situations, there can be lots of business logic exceptions or messages being created. In order to have the best user experience, we try to display detailed messages to the end user so if it's an error with some data, they know what they need to do to correct the situation. Sometimes this is through a message on a page, an email, etc. In some cases a generic message might be displayed and we log the message instead or send the message in an email to a 3rd party.

The service shouldn't need to care how the message is going to be handled, but just needs to create it in a way that it can be translated if needed.

Options
Translating when the message is created. There are two issues with this approach. Autowiring the translator interface to services that will display an error isn't ideal as it will increase the dependencies the service has. The larger issue is that because we would be throwing an exception that contains a string or passing a message that's a string (what the translator returns), if we have generic twig templates that run the translator function on a message, we're doing a double translation and the debug toolbar will indicate that there is a missing message and turn red. This makes finding missing translations much more difficult as we have to determine if there are any false positives and turns the red indicator into white noise.

Translating when displaying on a template. This option is possible, but adds complexity to our templates. Each time we display a message, we need to have some kind of logic that checks to see if it should be ran through the translator as-is or we need to get the data from the object and pass into the translator.

Proposed Solution
Adding a Phrase object that we can create and pass around reduces the template complexity and adds a nice way to create objects that can be translated at any point. This something that will be very familiar with Magento developers and other platforms also have similar solutions.

TODO

  • Determine if overwriting the parameters should be allowed (currently implemented this way)
  • Determine the name of the translatable class, currently Phrase as Message will likely get confused with Messenger
  • Determine if there should be a global function to allow quickly creating these. Similar to __() in Magento
  • Add docs
  • Add tests

Happy to get as much feedback as possible. I know a feature like this would be very welcomed by my team.

@linaori
Copy link
Contributor

linaori commented Jul 26, 2020

I like this implementation, primarily because you can add parameters to the message without having to define that elsewhere. Meaning you can return this object rather than having to return and array where the first key is the message, or a custom data container etc.

In the past I've built something similar where a process would create a similar object and then it translate this in another request:

{{ translatableMessage.key | trans(translatableMessage.parameters) }}

I like this idea because you push the dependency of translating to where it belongs; the view. I suspect this would also benefit translations within the form component where you can easily pass parameters if needed.

@ro0NL
Copy link
Contributor

ro0NL commented Jul 27, 2020

im wondering what type of objects would be "translatable", esp. in direct relation to a single message.

To me it hints for a value object instead, rather then a contract.

push the dependency of translating to where it belongs; the view

does this mean only the trans view layer should be translatable aware?

@linaori
Copy link
Contributor

linaori commented Jul 27, 2020

To me it hints for a value object instead, rather then a contract.

You'll need the contract to get the message key and parameters, possibly domain and language.

does this mean only the trans view layer should be translatable aware?

This is automatically the case if the translator can deal with it.

@ro0NL
Copy link
Contributor

ro0NL commented Jul 27, 2020

i meant interface. final class Translatable offers the same feature.

@linaori
Copy link
Contributor

linaori commented Jul 27, 2020

Can do that, though I can also see advantages of (serializable) custom objects.

@xabbuh xabbuh added this to the next milestone Jul 27, 2020
@natewiebe13
Copy link
Contributor Author

i meant interface. final class Translatable offers the same feature.

I like the idea of putting final on the implementation class in the PR. I do think we should still have an interface so we don't have to force a specific implementation, especially with making it final, but maybe I've misunderstood your comment.

One other example I'd like to provide, is at the end, it would be great to be able to use $this->addFlash('Successfully imported products.'); or $this->addFlash(__('Successfully imported %count% product(s)', ['%count%' => count($products])); and inside of the template just being able to translate all flash messages using {{ message|trans }}. It would remove complexity and having to really think too much about how the message is going to be used.

@ro0NL
Copy link
Contributor

ro0NL commented Jul 27, 2020

I do think we should still have an interface so we don't have to force a specific implementation

do you have any other implementation in mind? I dont see polymorphism being a requirement here.

Im not sure i follow your "addFlash" example, but it could account for Translatables as a feature.

Not sure about the __() example either, but see #35418 also. I think a generic t('id') funtion for extraction is nice yes, but also new Translatable('id') could be marked for extraction.

@natewiebe13
Copy link
Contributor Author

do you have any other implementation in mind? I dont see polymorphism being a requirement here.

I don't have any in mind at the moment, but potentially someone else might have a reason for them. We could just add a concrete class only, but though it might be a good to allow flexibility there. Users could also just use their own translator service to get around this limitation as well. Let me know if you want me to remove the interface.

Im not sure i follow your "addFlash" example, but it could account for Translatables as a feature.

In a basic CRUD setup, typically we'd add a flash message saying what was done and display it as a flash message on the next page. Using a translatable object would be great to avoid having to pass an already translated string and being able to just display on the next page without special handling or having Symfony think we're missing a translation, as we would like to just always translate the message in the view.

Not sure about the __() example either, but see #35418 also. I think a generic t('id') funtion for extraction is nice yes, but also new Translatable('id') could be marked for extraction.

__() is used in Magento [docs] and _e() in Wordpress [docs]. I believe these would be similar to t() like you mention in #35418. I think the leading underscore is a convention when creating translatable strings, so potentially _t() instead of just the t? Basically it would be a shortcut for new Translatable() as you mention.

I'm open to either way of dealing with this. Let me know what direction you'd like this to take.

@apfelbox
Copy link
Contributor

apfelbox commented Jul 28, 2020

i meant interface. final class Translatable offers the same feature.

We have built basically the same feature here: https://github.com/Becklyn/rad/blob/8.x/src/Translation/DeferredTranslation.php

We have some additional functionality, where not all of it makes sense here:

We have the concept that we either pass a string (not to-be-translated) or a DeferredTranslation.

  • We have named constructors: DeferredTranslation::messages($id), DeferredTranslation::backend($id), ...
  • We have a static ::translateValue($value) method, that either translates the Translatable or return the already pre-translated string.
  • We have a ::ensureValidValue() static method, that checks if the value is a either a string, Translatable or (optionally) null and automatically throws an UnexpectedTypeException.

The point is: from our POV the whole "translate this without using the translator" is a core feature and without these additional use cases it is really cumbersome to use.
But not all of these additional points make sense for Symfony core. So at least don't make the class final, so that one can add these features in their own lib / project.

PS: in terms of our DeferredTranslation, we have the same feature for DeferredRoute (create a URL without needing a dependency on the router, very frequently used) as well as DeferredForm (preparing and later manipulating the form config for ->createForm(...) without using any form service, only used in a single special case).

@linaori
Copy link
Contributor

linaori commented Jul 28, 2020

But not all of these additional points make sense for Symfony core. So at least don't make the class final, so that one can add these features in their own lib / project.

I don't see the added benefit of this. There's a very specific dataset you put in the data object. Handling these custom translations are not a part of the data object, nor should they be. If you need to do something custom, you can use composition (if the interface stays).

@apfelbox
Copy link
Contributor

If the interface stays, that would be fine as well.

Yeah, but for example things like merging parameters is (imo) a core feature. So if it's decided that this doesn't make it to the Translatable VO, then at least we need the interface. Otherwise I fear that is a super specific use case, that's not flexible enough for a lot of use cases.

There were quite a few of these in the past, so I'm just trying to chime in to avoid that here. Sorry for the noise.

@ro0NL
Copy link
Contributor

ro0NL commented Jul 28, 2020

Merging parameters y/n would be a Translator implementation detail :)

@natewiebe13
Copy link
Contributor Author

Hey @ro0NL, can you take another look at this? Once you're happy with where things are at, I think it would make sense to add a shorthand function (currently leaning towards _t()) and add some tests.

/**
* @return string The message id (may also be an object that can be cast to string)
*/
public function getMessageId(): string
Copy link
Member

Choose a reason for hiding this comment

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

I would use getMessage() as a message can be a plain English sentence or an id.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fabpot i proposed messageId. I think it's semantically more clear, as ultimately this is the ID value (even if it's a plain English sentence).

Copy link
Member

Choose a reason for hiding this comment

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

message is what is used everywhere in the component, we don't use message id anywhere, so I still think that message should be used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with this. Updated the PR.

@natewiebe13 natewiebe13 force-pushed the translatable-objects branch from 65ac028 to f6370da Compare August 11, 2020 17:44
@natewiebe13 natewiebe13 changed the title WIP: Translatable objects [Translator] Translatable objects Aug 15, 2020
@natewiebe13 natewiebe13 changed the title [Translator] Translatable objects [Translation] Translatable objects Aug 15, 2020
@natewiebe13 natewiebe13 requested a review from fabpot August 17, 2020 15:14
@fabpot
Copy link
Member

fabpot commented Aug 28, 2020

Can you rebase on current master (to remove the merge commit)?

@natewiebe13 natewiebe13 force-pushed the translatable-objects branch from 1b11bf8 to e08aebb Compare August 28, 2020 13:24
@natewiebe13
Copy link
Contributor Author

@fabpot rebased onto master

@fabpot fabpot force-pushed the translatable-objects branch from e08aebb to dc6b3bf Compare August 28, 2020 16:48
@fabpot
Copy link
Member

fabpot commented Aug 28, 2020

Thank you @natewiebe13.

@fabpot fabpot merged commit dc54cc9 into symfony:master Aug 28, 2020
@natewiebe13 natewiebe13 deleted the translatable-objects branch September 2, 2020 13:35
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Sep 25, 2020
This PR was squashed before being merged into the master branch.

Discussion
----------

Translatable objects

Goal: Add docs for translatable objects.

Feature: symfony/symfony#37670
Fixes #14145

Unsure how in-depth the docs should go. Open to feedback and collaboration.

Commits
-------

a2ad274 Translatable objects
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
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.