[Translation] XLIFF 2.0 read support #12853
Conversation
| */ | ||
| private function getVersion(\DOMDocument $dom) | ||
| { | ||
| $versionNumber = $this->getVersionNumber($dom); |
There was a problem hiding this comment.
You don't handle an UnexpectedValueException that might be thrown in getVersionNumber() neither here or in the load() method.
There was a problem hiding this comment.
Sure, my bad!
This should throw a InvalidArgumentException so we can catch on load method for better message readability.
Easy fix. Thank you!
bf7023b to
a8d0d81
Compare
| @@ -11,6 +11,8 @@ | |||
|
|
|||
| namespace Symfony\Component\Translation\Loader; | |||
|
|
|||
| use DOMDocument; | |||
| use InvalidArgumentException; | |||
There was a problem hiding this comment.
These use statements should be removed.
a8d0d81 to
9fc1da6
Compare
|
All comments by @xabbuh fixed, thanks for pointing out! |
9fc1da6 to
a1e5ac8
Compare
|
I don't like the current architecture using version classes:
And can you rebase your work on top of the current 2.7 branch ? It conflicts with the current codebase (which also means that tests will not run when updating your branch with new changes if conflicts are not fixed) |
a1e5ac8 to
0971c65
Compare
|
@xphere it is a good idea to comment when you update the PR, because we are not notified when new commits are added in a PR. Otherwise, there is a high chance that we won't review the PR again, just because we don't know there is a need for that. |
|
and can you rework your implementation based on my previous review ? |
|
@stof sure, sorry, I did rebase on 2.7 branch, but still need to relocate versions into extractor methods. Will do as soon as I've some spare time. |
|
@xphere any progress 😄 ? |
|
@xphere interested in updating this PR? If not I could try work on it. |
|
closed in favor of #15717 |
… aitboudad) This PR was merged into the 2.8 branch. Discussion ---------- [Translator][Loader] added XLIFF 2.0 support. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Fixed tickets | #11853, #12853 | Tests pass? | yes | License | MIT Commits ------- 0c24d55 [Translation][Dumper] added XLIFF 2.0 support. 7af4fc7 [XLIFF 2.0] added support for target attributes. ace6042 apply some fixes. ce540ae update changelog. ff5d6a3 [Translation][Loader] added XLIFF 2.0 support.
XliffFileLoader.versionorxmlnsattributes.Matches XLIFF 1.2 support, except that notes are not loaded now.
This is because they're not tied to a single translation (segment) anymore, but to a group of them (unit) or a whole file.