Skip to content
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

Add error message for XLIFF file load (#1627) #1640

Closed
wants to merge 1 commit into from

Conversation

anvit
Copy link
Contributor

@anvit anvit commented Jul 19, 2023

Throwing an exception when Symfony runs into issues when loading XLIFF files. This will help with catching broken XML files due to translation errors. Without any error handling, this would lead Symfony to use an empty array as the array of translations when it is unable to read XML files, and would make it delete all existing translations for such a file.

@anvit anvit added the Type: bug A flaw in the code that causes the software to produce an incorrect or unexpected result. label Jul 19, 2023
@anvit anvit added this to the 2.8.0 milestone Jul 19, 2023
@anvit anvit self-assigned this Jul 19, 2023
@anvit anvit linked an issue Jul 19, 2023 that may be closed by this pull request
@anvit anvit requested review from sbreker and a team July 19, 2023 22:01
@anvit
Copy link
Contributor Author

anvit commented Jul 19, 2023

This is one potential way to safeguard against this problem. Another could be to add a warning/exception in vendor/symfony/lib/i18n/sfMessageSource.class.php for scenarios where loadData doesn't return an array.

$data = &$this->loadData($source);
if (is_array($data))
{
$this->messages[$variant] = $data;
if ($this->cache)
{
$this->cache->set($variant.':'.$this->culture, serialize($data));
}
}

Decided to go with adding it here since generating the exception right when we're having an issue with loading the XLIFF file seemed like the more logical behaviour, but let me know if there's a better way, or a reason not to do this.

Copy link
Member

@mcantelon mcantelon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One suggestion but looks good otherwise!

$xmlErrors = libxml_get_errors();
$errorMessage = '';
foreach ($xmlErrors as $error) {
$errorMessage .= $error->message . '[File]: ' . $error->file .
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using sprinf to assemble the error message might be cleaner, but otherwise looks solid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@anvit anvit requested a review from mcantelon July 24, 2023 21:52
Copy link
Member

@mcantelon mcantelon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@anvit anvit force-pushed the dev/issue-1627-translations-xml-import branch from 86a2ca8 to dc16920 Compare September 15, 2023 16:58
Throwing an exception when Symfony runs into issues when loading XLIFF
files. This will help with catching broken XML files due to translation
errors. Without any error handling, this would lead Symfony to use an
empty array as the array of translations when it is unable to read XML
files, and would make it delete all existing translations for such a
file.
@anvit anvit force-pushed the dev/issue-1627-translations-xml-import branch from dc16920 to 7b7ffdb Compare September 15, 2023 17:08
@anvit anvit closed this Sep 15, 2023
@anvit anvit deleted the dev/issue-1627-translations-xml-import branch September 15, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bug A flaw in the code that causes the software to produce an incorrect or unexpected result.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate missing french translations in qtAccessionPlugin
2 participants