-
Notifications
You must be signed in to change notification settings - Fork 539
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OHRM5X-2480: Develop language package validation #1835
base: 5.7
Are you sure you want to change the base?
Conversation
73f3852
to
8c57ace
Compare
Use the sample template | ||
</oxd-text> | ||
</li> | ||
<li> | ||
<oxd-text class="orangehrm-information-card-text"> | ||
Only edit the target field. | ||
</oxd-text> | ||
</li> | ||
<li> | ||
<oxd-text class="orangehrm-information-card-text"> | ||
Do not change the template. | ||
</oxd-text> | ||
</li> | ||
<li> | ||
<oxd-text class="orangehrm-information-card-text"> | ||
Sample XLIFF : |
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.
We need to add these as lang strings. Check these references:
create .yaml and add lang strings: https://github.com/orangehrm/orangehrm/blob/main/installer/Migration/V5_6_0/lang-string/admin.yaml
add insert code to Migration.php: https://github.com/orangehrm/orangehrm/blob/main/installer/Migration/V5_6_0/Migration.php#L44-L49
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.
Added
rules: { | ||
attachment: [ | ||
required, | ||
maxFileSize(1048576), |
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.
@RajithaKumara we might have to implement some way to increase max file size in this release since we have had people asking before.
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.
check similar place and get as a config file
props: { | ||
label: 'Import', | ||
style: 'Text', | ||
displayType: 'text', |
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.
we will change this to an icon later
public const PARAMETER_LANGUAGE_ID = 'languageId'; | ||
public const PARAMETER_ATTACHMENT = 'attachment'; | ||
|
||
public const PARAM_RULE_IMPORT_FILE_FORMAT = ['text/xml', 'application/xml', 'application/xliff+xml']; |
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.
@TharakaAthukorala is it possible for .xlf
extension to take MIME types other than application/xliff+xml
If so we might have to apply sanitization here
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.
- The MIME type application/xliff+xml is the standard MIME type for .xlf files.
- For .xlf files, it's theoretically possible for other MIME types to be associated with this extension.
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.
.xliff
seems this also valid extension
public const PARAMETER_ATTACHMENT = 'attachment'; | ||
|
||
public const PARAM_RULE_IMPORT_FILE_FORMAT = ['text/xml', 'application/xml', 'application/xliff+xml']; | ||
public const PARAM_RULE_IMPORT_FILE_EXTENSIONS = ['xml', 'xlf']; |
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.
@RajithaKumara I feel we should only allow .xlf
. Tharaka has changed to export to give an .xlf
file as well. Not sure if we will have additional security concerns if we allow .xml
.
@@ -0,0 +1,158 @@ | |||
<?php |
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.
@TharakaAthukorala you need to add a new data group for this API in the Migration
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.
added
* @inheritDoc | ||
* @throws InvalidParamException | ||
* @throws \Exception |
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.
remove exceptions
* @inheritDoc | |
* @throws InvalidParamException | |
* @throws \Exception | |
* @inheritDoc |
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.
done
@@ -230,23 +233,26 @@ private function getXliffXmlSources(array $i18nGroups, I18NLanguage $language): | |||
$root->setAttribute('srcLang', 'en_US'); | |||
$root->setAttribute('trgLang', $language->getCode()); | |||
$root->setAttribute('xmlns', 'urn:oasis:names:tc:xliff:document:2.0'); | |||
$root->setAttribute('date', @date('Y-m-d\TH:i:s\Z')); | |||
// $root->setAttribute('date', @date('Y-m-d\TH:i:s\Z')); |
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.
date attribute is not allowed right? we can remove this line
https://docs.oasis-open.org/xliff/xliff-core/v2.0/xliff-core-v2.0.html
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.
removed
$document = new \DOMDocument(); | ||
$document->loadXML($content); | ||
|
||
if (null !== $targetLanguage = $this->getTargetLanguageFromFile($document)) { | ||
$normalizedLocalePattern = sprintf( | ||
'(%s|%s)', | ||
preg_quote($targetLanguage, '/'), | ||
preg_quote(str_replace('-', '_', $targetLanguage), '/') | ||
); | ||
// strict file names require translation files to be named '____.locale.xlf' | ||
$expectedFilenamePattern = sprintf('/^.*\.(?i:%s)\.(?:xlf|xliff)/', $normalizedLocalePattern); | ||
|
||
if (0 === preg_match($expectedFilenamePattern, basename($file))) { | ||
$errors[] = [ | ||
'line' => -1, | ||
'column' => -1, | ||
'message' => sprintf( | ||
'There is a mismatch between the language included in the file name ("%s") and the "%s" value used in the "target-language" attribute of the file.', | ||
basename($file), | ||
$targetLanguage | ||
), | ||
]; | ||
} | ||
} | ||
|
||
foreach (XliffUtils::validateSchema($document) as $xmlError) { | ||
$errors[] = [ | ||
'line' => $xmlError['line'], | ||
'column' => $xmlError['column'], | ||
'message' => $xmlError['message'], | ||
]; | ||
} | ||
|
||
libxml_clear_errors(); | ||
libxml_use_internal_errors($internal); | ||
|
||
return ['file' => $file, 'isValid' => 0 === \count($errors), 'messages' => $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.
just noting down that code taken from XliffLintCommand
in symfony/translations
https://github.com/symfony/translation/blob/5.4/Command/XliffLintCommand.php#L123-L156
We'll have to discuss this a bit. Not sure if we need the filename checking part.
Better to create a utility class for XliffUtils
for now. Reference: https://github.com/orangehrm/orangehrm/blob/main/src/plugins/orangehrmCorePlugin/Utility/Sanitizer.php
$units = $xliffDocument->getElementsByTagName('unit'); | ||
|
||
// Define validation constraints | ||
$maxLength = 255; |
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.
how did you decide this value?
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.
need to discuss the max char length for source and target XLIFFs. If there is no limit we can remove this length validation.
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.
API tests are pending
<div class="orangehrm-background-container"> | ||
<div class="orangehrm-card-container"> | ||
<oxd-text class="orangehrm-main-title"> | ||
Import Language Package: {{ languageName }} |
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.
<ul> | ||
<li> | ||
<oxd-text class="orangehrm-information-card-text"> | ||
Use the sample template. |
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.
</li> | ||
<li> | ||
<oxd-text class="orangehrm-information-card-text"> | ||
Only edit the target field. |
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.
</li> | ||
<li> | ||
<oxd-text class="orangehrm-information-card-text"> | ||
Do not change the template. |
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.
Handle these language strings
</li> | ||
<li> | ||
<oxd-text class="orangehrm-information-card-text"> | ||
Sample XLIFF : |
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.
public const PARAMETER_LANGUAGE_ID = 'languageId'; | ||
public const PARAMETER_ATTACHMENT = 'attachment'; | ||
|
||
public const PARAM_RULE_IMPORT_FILE_FORMAT = ['text/xml', 'application/xml', 'application/xliff+xml']; |
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.
.xliff
seems this also valid extension
|
||
$file = $xml->createElement('file'); | ||
$file->setAttribute('id', 1); |
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.
@devishke-orange check whether we have better id for this
@@ -270,4 +275,165 @@ private function getXliffXmlSources(array $i18nGroups, I18NLanguage $language): | |||
} | |||
return $xml; | |||
} | |||
|
|||
public function symfonyXliffValidations(string $content, ?string $file = null): array |
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.
add doc comment
$unitErrors = ""; | ||
|
||
// Validate source text | ||
if (strlen($source) > $maxLength) { |
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 (strlen($source) > $maxLength) { | |
if ($this->getTextHelper()->strLength($source, '8bit') > $maxLength) { |
} | ||
|
||
// Validate target text | ||
if (strlen($target) > $maxLength) { |
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.
strlen
80b16b5
to
38b3570
Compare
b2d99d3
to
dfc530e
Compare
…e unwanted langstrings
…erateCacheKey() into LocalizationService
No description provided.