Load original file metadata when loading Xliff 1.2 files #29148
Conversation
does it mean we have a spec violation? should we consider this a bug? |
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 |
f29c56b to
d3e586e
Compare
|
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? |
|
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.
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.
I think I recall having namespace issues without it.
e178ff1 to
4073319
Compare
|
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/a At 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 our 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 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.