Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Load original file metadata when loading Xliff 1.2 files#29148
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedNov 8, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
does it mean we have a spec violation? should we consider this a bug? |
eternoendless commentedNov 8, 2018
I guess it depends on where you draw the line regarding the scope of XliffFileLoader and MessageCatalogue. Is it just about loading the translatables and making them available for the Translator? Then it's not a bug. In my opinion, loading metadata like |
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM with one minor comment thanks.
fabpot left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Three things should be done before merging:
- Take Nicolas's comment into account
- Rebase on current master
- Add a note in the CHANGELOG.
f29c56b tod3e586eCompareeternoendless commentedJan 7, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Comment and rebase done 👍 I see that on the changelog, notes are under released versions. Should I add 4.3.0? Should I only add the note with no title? |
stof commentedJan 7, 2019
this should go in 4.3.0, yes |
| // If the xlf file has another encoding specified, try to convert it because | ||
| // simple_xml will always return utf-8 encoded values | ||
| $target =$this->utf8ToCharset((string) (isset($translation->target) ?$translation->target :$translation->source),$encoding); | ||
| $file->registerXPathNamespace('xliff',$namespace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think you need to register the namespace again. The registration on the root element should already be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think I recall having namespace issues without it.
e178ff1 to4073319Comparefabpot commentedJan 13, 2019
Thank you@eternoendless. |
…es (eternoendless)This PR was squashed before being merged into the 4.3-dev branch (closes#29148).Discussion----------Load original file metadata when loading Xliff 1.2 files| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aAt PrestaShop, we maintain our translations catalog automatically using an internal tool based on our [TranslationToolsBundle](https://github.com/PrestaShop/TranslationToolsBundle), which is capable of reverse building a MessageCatalogue by parsing the source code, and then saving it to Xliff files.Currently, this tool is only capable of building catalogs from scratch. We are currently moving to an incremental catalog where we only add new wordings, and keep old ones even if they are no longer present in the code (because of B/C). To do that, instead of starting from a clean MessageCatalogue, we load our current catalog using XliffLoader, and use that MessageCatalogue as a base. Easy peasy. But then we found a problem...The Xliff 1.2 standard defines a list of `<trans-unit>` elements within a collection of `<file>` elements. The `<file>` element has a required attribute named `original`, which is supposed to contain the name of the file where the wordings are used in (at least in our case it does). **This attribute is currently ignored by XliffFileLoader**.This means that it's currently impossible to read a Xliff 1.2 file using XliffFileloader, and save it back to Xliff without losing data.This Pull Request adds a new `file` element to the messages' metadata (alongside `notes`, `target-attributes` and `id`). Right now, it only contains `original`, but it could be extended to contain all the other attributes from the `<file>` element if needed.This required a small change in the loader where we loop through `<file>` elements before fetching their `<trans-unit>` children, instead of fetching all `<trans-unit>` elements at once.Commits-------4073319 Load original file metadata when loading Xliff 1.2 files
At PrestaShop, we maintain our translations catalog automatically using an internal tool based on ourTranslationToolsBundle, which is capable of reverse building a MessageCatalogue by parsing the source code, and then saving it to Xliff files.
Currently, this tool is only capable of building catalogs from scratch. We are currently moving to an incremental catalog where we only add new wordings, and keep old ones even if they are no longer present in the code (because of B/C). To do that, instead of starting from a clean MessageCatalogue, we load our current catalog using XliffLoader, and use that MessageCatalogue as a base. Easy peasy. But then we found a problem...
The Xliff 1.2 standard defines a list of
<trans-unit>elements within a collection of<file>elements. The<file>element has a required attribute namedoriginal, which is supposed to contain the name of the file where the wordings are used in (at least in our case it does).This attribute is currently ignored by XliffFileLoader.This means that it's currently impossible to read a Xliff 1.2 file using XliffFileloader, and save it back to Xliff without losing data.
This Pull Request adds a new
fileelement to the messages' metadata (alongsidenotes,target-attributesandid). Right now, it only containsoriginal, but it could be extended to contain all the other attributes from the<file>element if needed.This required a small change in the loader where we loop through
<file>elements before fetching their<trans-unit>children, instead of fetching all<trans-unit>elements at once.