Skip to content

[Translation] Make name attribute optional in xliff2 #40283

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 24, 2021
Merged

[Translation] Make name attribute optional in xliff2 #40283

merged 1 commit into from
Feb 24, 2021

Conversation

MarieMinasyan
Copy link
Contributor

@MarieMinasyan MarieMinasyan commented Feb 23, 2021 •
edited
Loading

Do not set a fake name attribute on unit element from xliff2 to allow using source attribute and avoid missing translation error

Q A
Branch? 5.2
Bug fix? yes/no
New feature? no
Deprecations? no
Tickets Fix #37055
License MIT
Doc PR symfony/symfony-docs#...

When xlf translations are loaded, if a name exists on unit element, the segment's source is ignored:

            foreach ($unit->segment as $segment) {
                $attributes = $unit->attributes();
                $source = $attributes['name'] ?? $segment->source;

At the same time, when dumping translations, the segment's source is copied into the unit's name attribute, unless it's longer than 80 characters. In that case, substr(md5($source), -7) is set into the name attribute.
This results in a missing translation error, because the source is ignored and the name is a random string.

Suggested solution: only set the name attribute if the string is less than 80 characters.

@derrabus derrabus mentioned this pull request Feb 23, 2021
Closed
@derrabus derrabus added this to the 5.2 milestone Feb 23, 2021
@derrabus derrabus added the Bug label Feb 23, 2021
nicolas-grekas
nicolas-grekas reviewed Feb 24, 2021
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Should this apply to branch 4.4?
Related PR: #26661
/cc @Nyholm I think you might be the most knowledgeable reviewer on this one 🙏

@Nyholm
Copy link
Member

Nyholm commented Feb 24, 2021 •
edited
Loading

This feature was added in #26149, I am very sparse on details why this was added. But it is added according to the XLIFF specification. I assume it was to be compatible with parsers and external systems.

@Taluu updated the loader in #35373. He is equally short on details why. Maybe you could elaborate?

To fix this issue, we should either revert parts of #35373 or merge this PR in its current state.

For future reference: The name is optional on the unit.

@MarieMinasyan
Copy link
Contributor Author

Hi @nicolas-grekas! No problem, I can move it to branch 4.4, just let me know.

@Taluu
Copy link
Contributor

Taluu commented Feb 24, 2021

@Taluu updated the loader in #35373. He is equally short on details why. Maybe you could elaborate?

From what I can remember, translation tools such as memsource, phrase app and probably others are not technical, but aimed for translation teams.

As such, <source> elements should not contain "keys" such as foo.bar.baz but the original sentence such as The foo is also bar or baz : the tool then helps to translate this into the targeted language. It can't proceed with "keys".

From what I saw in the xliff1 loader and bits from the spec, it seems this is the role of the name attribute on the <unit> element.

To fix this issue, we should either revert parts of #35373 or merge this PR in its current state.

For future reference: The name is optional on the unit.

That is why I set the priority on the name then the source. We could probably change this so there's a key duality (element accessible on both name and source value) ? But this could be heavy imo...

I think this is mergeable as is. Or another way to look at it would be to not try to write a name attribute.

@Nyholm
Copy link
Member

Nyholm commented Feb 24, 2021

So, source should be "My original string". The name should be the "foo.bar.baz" translation key. The name is optional in the specification.

We don't know if the end users is using translation keys or original stings in their source code. So what we say is:

Loader: If name exists, we think it is a translation key, if not, we treat the source as translation key.
Dumper: If the $source is short (<80 chars), we think it is a translation key and add it to name.

As pointed out before $source could be any kind of complex string. That is why we added the 80 char limit. Why 80 and not 60 or 100? I dont know...

For the record: The Loader and Dumper must be compatible, but you cannot assume that a .xlf file was written with our Dumper.

Im happy with this PR.

👍

Nyholm
Nyholm approved these changes Feb 24, 2021
View reviewed changes
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you.

I like that you added tests. Well done.

I consider this a bug fix, could you edit the PR to change the base to 4.4?

@nicolas-grekas nicolas-grekas modified the milestones: 5.2, 4.4 Feb 24, 2021
@MarieMinasyan @nicolas-grekas
[Translation] Make name attribute optional in xliff2
9705855 
Do not set a fake `name` attribute on `unit` element from xliff2 to allow using `source` attribute and avoid missing translation error
@nicolas-grekas nicolas-grekas changed the base branch from 5.2 to 4.4 February 24, 2021 16:01
nicolas-grekas
nicolas-grekas approved these changes Feb 24, 2021
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I rebased on 4.4)
Thanks @Nyholm for the review!

@nicolas-grekas
Copy link
Member

Thank you @MarieMinasyan.

@nicolas-grekas nicolas-grekas merged commit 91121ac into symfony:4.4 Feb 24, 2021
@MarieMinasyan MarieMinasyan deleted the fix-issue-37055 branch February 24, 2021 16:32
This was referenced Mar 4, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@Nyholm Nyholm Nyholm approved these changes

@chalasr chalasr Awaiting requested review from chalasr

@dunglas dunglas Awaiting requested review from dunglas

@jderusse jderusse Awaiting requested review from jderusse

@lyrixx lyrixx Awaiting requested review from lyrixx

@sroze sroze Awaiting requested review from sroze

@wouterj wouterj Awaiting requested review from wouterj

@xabbuh xabbuh Awaiting requested review from xabbuh

@yceruto yceruto Awaiting requested review from yceruto

Assignees
No one assigned
Labels
Bug Translation
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
5 participants
@MarieMinasyan @Nyholm @Taluu @nicolas-grekas @derrabus