Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Translation] Improved the performance of the lint:xliff command#27653
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 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.
some random comments :)
| $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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ($errors = XliffUtils::validateSchema($dom)) {
| } | ||
| libxml_use_internal_errors(true); | ||
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.
should be reverted and maybe done on 2.8/3.4?
| foreach (libxml_get_errors()as$xmlError) { | ||
| foreach (XliffUtils::validateSchema($document)as$xmlError) { | ||
| $errors[] =array( | ||
| 'line' =>$xmlError->line, |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suggest tagging this class as@internal for now
| if (!$isValid) { | ||
| libxml_disable_entity_loader($disableEntities); | ||
| returnstatic::getXmlErrors($internalErrors); |
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.
accessing private static methods must be done usingself::, notstatic. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
shouldn't we also reset the usage of internal errors before returning here ?
We could even usetry/finally for that to ensure it always happen.
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'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; | ||
| } | ||
| privatestaticfunctiongetSchema($xliffVersion):string |
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.
string typehint ?
| foreach ($xmlErrorsas$error) { | ||
| $errorsAsString .=sprintf("[%s %s] %s (in %s - line %d, column %d)\n", | ||
| LIBXML_ERR_WARNING ==$error['level'] ?'WARNING' :'ERROR', |
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.
strict comparison should be used
javiereguiluz commentedJun 20, 2018
Fixed everything. Thanks for the review. |
fabpot commentedJun 20, 2018
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!