[Translation] Improved the performance of the lint:xliff command #27653
Conversation
| $this->validateSchema($xliffVersion, $dom, $this->getSchema($xliffVersion)); | ||
| $xliffVersion = XliffUtils::getVersionNumber($dom); | ||
| $errors = XliffUtils::validateSchema($dom); | ||
| if (0 !== count($errors)) { |
There was a problem hiding this comment.
if ($errors = XliffUtils::validateSchema($dom)) {
| @@ -110,7 +111,6 @@ private function validate($content, $file = null) | |||
| } | |||
|
|
|||
| libxml_use_internal_errors(true); | |||
|
|
|||
There was a problem hiding this comment.
should be reverted and maybe done on 2.8/3.4?
| $errors[] = array( | ||
| 'line' => $xmlError->line, |
There was a problem hiding this comment.
indentation fix worth a PR on 2.8/3.4 I suppose also
| * Provides some utility methods for XLIFF translation files, such as validating | ||
| * their contents according to the XSD schema. | ||
| * | ||
| * @author Javier Eguiluz <javier.eguiluz@gmail.com> |
There was a problem hiding this comment.
not sure about this sorry :(
| { | ||
| /** | ||
| * Gets xliff file version based on the root "version" attribute. | ||
| * Defaults to 1.2 for backwards compatibility. |
There was a problem hiding this comment.
there should be an empty phpdoc line after the short description
| * Provides some utility methods for XLIFF translation files, such as validating | ||
| * their contents according to the XSD schema. | ||
| * | ||
| * @author Javier Eguiluz <javier.eguiluz@gmail.com> |
There was a problem hiding this comment.
I suggest tagging this class as @internal for now
| if (!$isValid) { | ||
| libxml_disable_entity_loader($disableEntities); | ||
|
|
||
| return static::getXmlErrors($internalErrors); |
There was a problem hiding this comment.
accessing private static methods must be done using self::, not static. Otherwise, when extending the class, PHP tries to access it using the child class, which won't work (same for all other static calls to a private method)
|
|
||
| $isValid = @$dom->schemaValidateSource(static::getSchema($xliffVersion)); | ||
| if (!$isValid) { | ||
| libxml_disable_entity_loader($disableEntities); |
There was a problem hiding this comment.
shouldn't we also reset the usage of internal errors before returning here ?
We could even use try/finally for that to ensure it always happen.
There was a problem hiding this comment.
I'm not sure about this. I'm using the exact same code as before. I'd prefer to not change that logic because it's unrelated to this PR.
| return $errorsAsString; | ||
| } | ||
|
|
||
| private static function getSchema($xliffVersion): string |
|
|
||
| foreach ($xmlErrors as $error) { | ||
| $errorsAsString .= sprintf("[%s %s] %s (in %s - line %d, column %d)\n", | ||
| LIBXML_ERR_WARNING == $error['level'] ? 'WARNING' : 'ERROR', |
|
Fixed everything. Thanks for the review. |
|
Thank you @javiereguiluz. |
…ff command (javiereguiluz) This PR was squashed before being merged into the 4.2-dev branch (closes #27653). Discussion ---------- [Translation] Improved the performance of the lint:xliff command | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27564 | License | MIT | Doc PR | - As suggested by @stof I extracted the schema validation logic from XliffFileLoader to reuse it in the `lint:xliff` command. The validation is now instantaneous, so the command is blazing fast! Commits ------- e53bf58 [Translation] Improved the performance of the lint:xliff command
This PR was merged into the 4.1 branch. Discussion ---------- [Translation] fix perf of lint:xliff command | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27564 | License | MIT | Doc PR | - #27653 has been merged on master as an improvement, but the perf issue is a killer. Our CI spends 1 minutes on just a few translation test cases. Only 4.1 has this behavior. That's a bug. Commits ------- 02c69b1 [Translation] fix perf of lint:xliff command
As suggested by @stof I extracted the schema validation logic from XliffFileLoader to reuse it in the
lint:xliffcommand. The validation is now instantaneous, so the command is blazing fast!