Problem/Motivation

When XLIFF is imported, there's a certain validation phase.
If you see at method validateImport(), there's tons of abort conditions that clearly test specific abort cases, but don't provide a meaningful message.

This leads to aborts with just messages like "Failed to validate file, import aborted."

Proposed resolution

For every condition to abort, add a specific error message.

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#34 xliff_validation_fail-2278093-34-interdiff.txt610 bytessasanikolic
#34 xliff_validation_fail-2278093-34.patch7.11 KBsasanikolic
#30 xliff_validation_fail-2278093-30-interdiff.txt610 bytessasanikolic
#30 xliff_validation_fail-2278093-30.patch45.43 KBsasanikolic
#28 xliff_validation_fail-2278093-28.patch7.71 KBsasanikolic
#21 xliff_validation_messages-2278093-21.patch7.31 KBmiro_dietiker
#19 xliff_validation_messages-2278093-16.patch6.78 KBmiro_dietiker
#18 xliff_validation_messages-2278093-14.patch7.86 KBmiro_dietiker
#14 xliff_validation_messages-2278093-14.patch7.86 KBmiro_dietiker
#12 xliff_validation_messages-2278093-12.patch7.86 KBmiro_dietiker
#10 xliff_validation_messages-2278093-10.patch7.74 KBblueminds
#9 xliff_validation_messages-2278093-9.interdiff.txt3.68 KBblueminds
#9 xliff_validation_messages-2278093-9.patch3.68 KBblueminds
#7 xliff_validation_messages-2278093-7.patch6.95 KBmiro_dietiker
#4 xliff_validation_messages-2278093-3.patch2.2 KBblueminds
#2 xliff_validation_messages-2278093-2.patch1.63 KBblueminds
#1 xliff_validation_messages-2278093-1.patch1.61 KBblueminds
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

blueminds’s picture

blueminds CreditAttribution: blueminds commented
Status: Active » Needs review
FileSize
xliff_validation_messages-2278093-1.patch1.61 KB

Messages added.

blueminds’s picture

blueminds CreditAttribution: blueminds commented
FileSize
xliff_validation_messages-2278093-2.patch1.63 KB

Just realized that the message type was not set.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Needs review » Needs work 
+++ b/translators/file/tmgmt_file.format.xliff.inc
@@ -209,27 +209,32 @@ class TMGMTFileformatXLIFF extends XMLWriter implements TMGMTFileFormatInterface
+      drupal_set_message(t('Unable to match the translation job based on the data from the file.'), 'error');
...
+      $job->addMessage('The source language from the file does not match the job source language.', array(), 'error');
...
+      $job->addMessage('The target language from the file does not match the job target language.', array(), 'error');

I do like to see the (job id, language) values in the message.

blueminds’s picture

blueminds CreditAttribution: blueminds commented
Status: Needs work » Needs review
FileSize
xliff_validation_messages-2278093-3.patch2.2 KB

yup

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Related issues: +#2278093: XLIFF validation fail without more information

Oops, found some UI issue.

blueminds’s picture

blueminds CreditAttribution: blueminds commented

That is this one.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
FileSize
xliff_validation_messages-2278093-7.patch6.95 KB

Providing new patch with way improved error handling. Untested...

Added new optional $job param to validateImport and match the Job ID. Fixing also #2278093: XLIFF validation fail without more information

Will test in production and apply if it works later.

Status: Needs review » Needs work

The last submitted patch, 7: xliff_validation_messages-2278093-7.patch, failed testing.

blueminds’s picture

blueminds CreditAttribution: blueminds commented
Status: Needs work » Needs review
FileSize
xliff_validation_messages-2278093-9.patch3.68 KB
xliff_validation_messages-2278093-9.interdiff.txt3.68 KB 
+++ b/translators/file/tmgmt_file.format.html.inc
@@ -84,11 +83,26 @@ class TMGMTFileFormatHTML implements TMGMTFileFormatInterface {
+    if (empty($job)) {

This condition made the inner checks skip if the job was provided. Being able to validate we should make the job argument compulsory so the tjid checks can be carried out.

blueminds’s picture

blueminds CreditAttribution: blueminds commented
FileSize
xliff_validation_messages-2278093-10.patch7.74 KB

the patch is the same as interdiff... sry

The last submitted patch, 9: xliff_validation_messages-2278093-9.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
FileSize
xliff_validation_messages-2278093-12.patch7.86 KB

No, making it mandatory changes the interface and this is not correct - see drush usage that doesn't provide it!
Providing improved version with your test fixes. Thank you.

Status: Needs review » Needs work

The last submitted patch, 12: xliff_validation_messages-2278093-12.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
FileSize
xliff_validation_messages-2278093-14.patch7.86 KB

Fixing again ;-)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Needs work » Needs review

Aww i need to write more patches to get back again into the flow. ;-)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/translators/file/tmgmt_file.format.interface.inc
@@ -19,14 +19,20 @@ interface TMGMTFileFormatInterface {
    *
-   * @param $imported_file
+   * @todo this function should NOT return a job. We need a import processor
+   *   instance instead to deal with the import context.
+   *

I'm not sure what this and the related changes have to do with this issue?

The idea of this process was to support a global import translation form that can find out the job it applies to automatically and also support uploading multiple translations at once and so on.

Or the existing drush integration, which also gets the job from the file.

Sure this could be split into a separate method but finding out the job it applies to is validation too (checking if it is actually a xliff file that we generated with the necessary metadata to identify the job).

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

We discussed this a bit.

The main problem is that the @todo and the added argument/job-is-the-same validation go in a different direction. If we would implement the @todo and separate the "get the job" part in a separate method, then keeping that part of the validation inside the validate method would no longer sense and we'd have to change it again.

Discussed this with Miro, we decided to keep the @todo (a reference to an issue would be nice there, can be a dummy) and instead move the "job is the same" part of the validation into the form validate callback that invokes the validate method. That' more future proof and makes this part of an internal implementation instead of a public API.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
FileSize
xliff_validation_messages-2278093-14.patch7.86 KB

As discussed, added new @todo about addMessage and unsafe Context.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
FileSize
xliff_validation_messages-2278093-16.patch6.78 KB

Ooops, wrong patch :-!

Status: Needs review » Needs work

The last submitted patch, 19: xliff_validation_messages-2278093-16.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Needs work » Needs review
FileSize
xliff_validation_messages-2278093-21.patch7.31 KB

Niiiice, retry fixing test.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

Yay! Committed!
Followup is #2278997: Create file import processor to moderate job

blueminds’s picture

blueminds CreditAttribution: blueminds commented

If committed shouldn't the status be updated as well?

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Needs review » Fixed

Thx, i thought i did.

  • Commit 824de61 on 7.x-1.x by miro_dietiker: 
    Issue #2278093 by miro_dietiker, blueminds: Fixed XLIFF validation fail...
miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Fixed » Patch (to be ported)

Uh, needs port to 8.x

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Issue tags: +XLIFF
sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic commented
Status: Patch (to be ported) » Needs review
FileSize
xliff_validation_fail-2278093-28.patch7.71 KB

Ported to 8.x with a simple test.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented 
+++ b/translators/tmgmt_file/src/Format/FormatInterface.php
@@ -35,7 +35,7 @@ interface FormatInterface {
-  function validateImport($imported_file);
+  public function validateImport($imported_file);

methods in an interface area always public, it's possible that we did this in the 7.x patch, but it's not necessary.

sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic commented
FileSize
xliff_validation_fail-2278093-30.patch45.43 KB
xliff_validation_fail-2278093-30-interdiff.txt610 bytes

Ok, removed the public definition of the method validateImport.

  • Berdir committed 274fc2b on 8.x-1.x authored by sasanikolic 
    Issue #2278093 by miro_dietiker, blueminds, sasanikolic: Fixed XLIFF...
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

Arg, accidently committed this. Patch has not been rerolled properly and is reverting a lot of stuff.

  • Berdir committed 270b18f on 8.x-1.x 
    Revert "Issue #2278093 by miro_dietiker, blueminds, sasanikolic: Fixed...
sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic commented
Status: Needs work » Needs review
FileSize
xliff_validation_fail-2278093-34.patch7.11 KB
xliff_validation_fail-2278093-34-interdiff.txt610 bytes

Sorry about that. Should be fine now...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Fixed

Better :)

  • Berdir committed c135782 on 8.x-1.x authored by sasanikolic 
    Issue #2278093 by miro_dietiker, blueminds, sasanikolic: XLIFF...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.