Skip to content

Translations in di.xml #223

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

buskamuza
Copy link
Contributor

@buskamuza buskamuza commented Aug 1, 2019

Problem

There are translate attribute supported in layouts, while di.xml supports translatable with different behavior. This is confusing and may lead to bugs.

See MAGETWO-67048 (internal issue with old discussion).

Solution

Unify translation attributes in XML files.

Requested Reviewers

@melnikovi
@maghamed
@kokoc

1. Allow `translate` attribute in `di.xml`.
2. `translate` attribute leads to the string being automatically translated during XML parsing and passed to the client code as translated phrase.
2.1. If it is impossible to translate the phrase (e.g., the application is running in `global` area), the phrase is provided untranslated.
3. Remove `translatable` attribute. Temporarily, support it as deprecated with existing behavior.
Copy link
Contributor

Choose a reason for hiding this comment

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

This would remove the ability to defer the translation. Is this an acceptable feature loss?

@melnikovi melnikovi self-assigned this Aug 5, 2019
@buskamuza buskamuza mentioned this pull request Aug 7, 2019
@buskamuza
Copy link
Contributor Author

  • @kandy : keep behavior as is. It may be impossible to implement due to di.xml being loaded too early
  • Look into all cases where translations are provided via di.xml and see if we can eliminate translations from di.xml at all.

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.

4 participants