Change record: mask HTML markup in XLIFF

> Since i don't know the proper implementation status of
> this feature in XLIFF compatible software, we'll need to check the adaption.

Here are some notes that may help:

There are only in a few rare cases of XLIFF-aware tools that may not understand the inline codes elements such as <bpt>/<ept>/<ph>/etc. The wide majority handle them properly.

Some tend to put all original codes in <ph>. But if you can make the distinction between opening/closing/placeholder types (e.g. <b>, </b>, <br/>) I would strongly recommend to use the proper XLIFF element for each type.

You can also use <g>...</g> and <x/> if you don't want to include the original code in the XLIFF document. But your merging tool will then need to know how to put the original codes back.

I also strongly recommend to handle segmented entries properly when merging back: by stripping <mrk mtype='seg'> elements' tags (not their content). Other <mrk> elements may also be present that you may want to handle to either feed them back into the content (e.g. a comment) or strip them out.

Quick input from our experience:
Our translator uses SDL Trados Studio 2011 which is not able to leave the html tags out of the translation tasks by default. This means that the translator can not do a proper word count and that makes it difficult for them to make a quote that matches the work that is done or has to be done.

Another thing is that the html tags are counted as words for a translation job inside the module itself. So this means that whenever the translator has a system that does leave out the html-tags you don't have a correct wordcount of the translation job to double-check the translator, which is pretty important as well.

So this would be a nice feature to have inside the tmgmt module itself as far as we are concerned. A nice 'should have' so to say.

- Provides a way to add "processors". A processor can take an action upon translation request and xliff import.
- The patch also includes html masking processor that will mask and unmask the html using and tags.

Installed the patch and tested.

When the 'Escape Html' box is checked in the translator settings, the job is masked with bpt and ept markup. Unfortunately, I do not have a program available to import the result to. So I cannot analyse the result.

Shouldn't the 'Escape HTML' setting not be available on the job level?

When the 'Escape HTML' box is NOT checked, the job does not come back after 'Submit to translator' (blank page)

BTW: It is super stupid that the preview for issues shows only the original body (which never changes) and not the comment (which is the only thing that changes)....progress!

That sounds pretty good. Can't wait to test it in a new release...

@Rob

Can you please test the masking in Trados? You can test the patched version by clicking on this link:

http://simplytest.me/project/tmgmt/7.x-1.x?patch[]=https://drupal.org/fi...

Then choose 'Launch Sandbox' which will setup a tmgmt Site with the patch applied.

Or you apply the patch onto you own system.

Please do not forget to check the box 'Escape Html' in the translator's settings.

@Rob1106 will you be able to test the patch? I do not have a tool to test it in real life, just followed specs. It needs to be tested in real life before we push it forward.

Tested the attached patch with Trados studio. The basic workflow works, still needs improvements and fixing test coverage, however ready for trial ride and feedback.

Attaching patch hopefully solving all issues. Ready for review.

Added documentation, import integrity check and removed two testing html files and instead moved testing HTML into single sample file.

The current status is that it works with Trados quite nice. Probably would make sense to try it out with different tool as well.

There is still one issue though, and that is img attributes - they are added as attributes of the placeholder element
without x-html: prefix. This is due to fact it fails validation in Trados then. Tried it out with xml: prefix, and that worked, but not sure if it is correct.

comments implemented

here we go

First review.

Haven't reviewed everything yet, code looks quite good, but I'm a bit worried about complexity/the overall thing, how this new plugin type plays together with the different file formats and it's kind of XLIFF specific atm but then again not and it also kind of overlaps with the new escape feature that we introduced recently for translatable string placeholder escaping...

1. +++ b/tests/tmgmt_test.module
   @@ -51,3 +51,15 @@ function tmgmt_test_tmgmt_source_suggestions(array $items, TMGMTJob $job) {
   +/**
   + * Implements hook_tmgmt_fle_text_processor_plugin_info().
   

   typo: fle

2. +++ b/tests/tmgmt_test.text_processor.inc
   @@ -0,0 +1,80 @@
   +   * Will update translation text.
   

   first word should be third tense.

   Updates translation text.

   Should be enough, no need for will?

3. +++ b/tests/tmgmt_test.text_processor.inc
   @@ -0,0 +1,80 @@
   +  protected function updateTranslation(&$data) {
   +    if (isset($data['#translation'])) {
   +      $data['#translation']['#text'] = str_replace('{processed}', '', $data['#translation']['#text']);
   +    }
   +    else {
   +      foreach (element_children($data) as $key) {
   +        $this->updateSourceText($data[$key]);
   +      }
   +    }
   +  }
   

   Shouldn't this call itself not the other method?

   Makes me wonder why the tests aren't failing ;)

4. +++ b/translators/file/tmgmt_file.format.xliff.inc
   @@ -50,11 +50,11 @@ class TMGMTFileformatXLIFF extends XMLWriter implements TMGMTFileFormatInterface
        $this->writeAttribute('xml:lang', $this->job->getTranslator()->mapToRemoteLanguage($this->job->target_language));
   -    $this->text($element['#text']);
   +    $this->writeRaw(!empty($element['#translation']['#text']) ? $element['#translation']['#text'] : $element['#text']);
        $this->endElement();
   

   I don't remember why I added the source also as translation, does that actually make sense? :)

5. +++ b/translators/file/tmgmt_file.format.xliff.inc
   @@ -135,7 +135,12 @@ class TMGMTFileformatXLIFF extends XMLWriter implements TMGMTFileFormatInterface
        $data = array();
        foreach ($xml->xpath('//xliff:trans-unit') as $unit) {
   -      $data[(string) $unit['id']]['#text'] = (string) $unit->target;
   +      // Load the result with XMLReader so that we are able to get the inner
   +      // xml.
   +      $reader = new XMLReader();
   +      $reader->XML($unit->target->asXML());
   +      $reader->read();
   +      $data[(string) $unit['id']]['#text'] = $reader->readInnerXml();
        }
        return tmgmt_unflatten_data($data);
   

   This looks quite inefficient, can we at least re-use the same class?

6. +++ b/translators/file/tmgmt_file.module
   @@ -32,37 +32,92 @@ function tmgmt_file_theme() {
   +      try {
   +        $controller_class = tmgmt_file_text_processor_controller($name);
   +        /** @var TMGMTFileTextProcessorInterface $processor_controller */
   +        $processor_controller = new $controller_class;
   +        $processor_controller->validateJobTranslationUponImport($job, $file->uri);
   +      }
   +      catch (TMGMTException $e) {
   +        $job->addMessage($e->getMessage(), array(), 'error');
   +        $validated_job = FALSE;
   +        break;
   +      }
   

   Might be useful to have tests for the validation failure part here, haven't seen the tests yet but the test implementation is empty.

7. +++ b/translators/file/tmgmt_file.module
   @@ -32,37 +32,92 @@ function tmgmt_file_theme() {
   +  if (!$validated_job) {
   +    $job->addMessage('Failed to validate file, import aborted.', array(), 'error');
   +    return FALSE;
   +  }
   +  elseif ($validated_job->tjid != $job->tjid) {
   +    $uri = $validated_job->uri();
   +    $label = $validated_job->label();
   +    $job->addMessage('Import file is from job <a href="@url">@label</a>, import aborted.', array('@url' => url($uri['path']), '@label' => $label));
   +    return FALSE;
   +  }
   +  else {
   

   You have both if/elseif/else *and* return FALSE.

   I think it would be easier to read if you just do return FALSE and have if () return FALSE; if (return FALSE) ... less nesting.

8. +++ b/translators/file/tmgmt_file.module
   @@ -109,6 +167,43 @@ function tmgmt_file_format_controller($plugin = NULL) {
   + *
   + * @return TMGMTFileTextProcessorInterface
   + *   Either a specific text processor plugin controller instance or an array of
   + *   available controllers.
   + */
   +function tmgmt_file_text_processor_controller($plugin = NULL) {
   +  return _tmgmt_plugin_controller('file_text_processor', $plugin);
   +}
   

   Probably copied, but that's not true according to how it's used? there it's a string that's used as new $bla.

9. +++ b/translators/file/tmgmt_file.plugin.inc
   @@ -29,6 +29,17 @@ class TMGMTFileTranslatorPluginController extends TMGMTDefaultTranslatorPluginCo
   +      // If we have processors do the source text processing.
   +      if ($text_processors = $job->getTranslator()->getSetting('text_processor_plugins')) {
   +        foreach ($text_processors as $name) {
   +          $controller_class = tmgmt_file_text_processor_controller($name);
   +          /** @var TMGMTFileTextProcessorInterface $controller */
   +          $controller = new $controller_class;
   +          $controller->processJobSourceUponRequest($job);
   +        }
   +      }
   +
   

   Isn't processing tightly coupled with the file format? XLIFF-transformation for HTML export isn't very useful :)

   I know, you just aren't supposed to select it, but that's not exactly great UX :)

10. +++ b/translators/file/tmgmt_file.recursive_iterator.inc
    @@ -0,0 +1,93 @@
    + * Class used to iterate through DOMDocument().
    

    DOMDocument is a class, not a function, so no ().

11. +++ b/translators/file/tmgmt_file.recursive_iterator.inc
    @@ -0,0 +1,93 @@
    +  /**
    +   * Current Position in DOMNodeList
    +   * @var Integer
    +   */
    

    position small, . at the end and missing empty line before @var

12. +++ b/translators/file/tmgmt_file.recursive_iterator.inc
    @@ -0,0 +1,93 @@
    +   *   DOMNode to itterate over.
    

    t-to many t-t's :)

13. +++ b/translators/file/tmgmt_file.recursive_iterator.inc
    @@ -0,0 +1,93 @@
    +   * @return DOMNode
    ...
    +   * @return RecursiveDOMIterator
    ...
    +   * @return Boolean
    ...
    +   * @return Integer
    

    int/bool, missing descriptions.

14. +++ b/translators/file/tmgmt_file.test
    @@ -19,11 +19,185 @@ class TMGMTFileTestCase extends TMGMTBaseTestCase {
       function setUp() {
    -    parent::setUp(array('tmgmt_file', 'tmgmt_ui'));
    +    parent::setUp(array('tmgmt_file', 'tmgmt_ui', 'tmgmt_test'));
    

    tmgmt_test is always enabled in the base class.

15. +++ b/translators/file/tmgmt_file.test
    @@ -19,11 +19,185 @@ class TMGMTFileTestCase extends TMGMTBaseTestCase {
       }
     
    +  function testTextProcessorAPI() {
    

    Missing docblock.

16. +++ b/translators/file/tmgmt_file.text_processor.interface.inc
    @@ -0,0 +1,52 @@
    +  /**
    +   * Will process job source text.
    +   *
    +   * The implementation should do whatever necessary to process the source text.
    +   * Upon performing needed changes the implementation should take care of
    +   * persisting these changes.
    

    "Will" also unnecessary here.

    I don't really understand the last sentence, persist?

Closed review window accidently in the middle, will continue later

About the escape problem, the idea of the API there was that knowing what should be escaped and knowing how it needs to be escaped are two different things and we tried to separate it there, but then again the way html is escaped in xliff is connected, so that might not work well.

Don't have a good solution atm...

+++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
@@ -0,0 +1,381 @@
+ * To process the content DOMDocument object is used due to its ability to
+ * read broken HTML. This also implies that if broken HTML is in the source
+ * content the translation content will be fixed into the extend of DOMDocument
+ * abilities.

What exactly is source and translation content here? My understanding is that source is the actual node and fields and translation content is both the source text that we initially process and the translation that we get back, as both go through DOMDocument?

Wherever we make the split, we end up with a mismatch as we can't fix the actual source, be it our review form or the actual sources.

We can't change this, but maybe we should make this a bit more explicit, e.g. in the form of a description.

More tomorrow

1. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
   @@ -0,0 +1,381 @@
   +      $job_item->data = $data;
   

   Not sure if this can really be considered a public property and should be set like this, it happens to be public because the storage controller needs it.

   We have updateData(), but that's rather slow, not sure.

2. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
   @@ -0,0 +1,381 @@
   +   */
   ...
   +  /**
   +   * Will check the import integrity.
   +   *
   +   * It will count all sub elements of the trans-unit/target element and compare
   +   * it with the value stored during export.
   +   *
   +   * {@inheritdoc}
   

   @inheritdoc can't be mixed with additional comments. api.module doesn't support that.

   Instead of the second paragraph, you could move that inline and also add some more comments what you're doing there. The first seems unecessary. Also more uneccessary "will"'s :)

3. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
   @@ -0,0 +1,381 @@
   +
   +    foreach ($xml->xpath('//xliff:trans-unit') as $unit) {
   +      $count = 0;
   +      $reader = new XMLReader();
   ...
   +      }
   

   Would be important to test this with multiple job items and data items I think, to make sure that this logic here works correctly.

   Also again the mix of simplexml and XMLReader. Again should probably at least re-use the reader (if that is actually faster, not sure), the more performant way would probably be to only use one API. Not sure how feasable that is.

   All this is doing is counting opening tags, right (comments! :))

4. +++ b/translators/file/tmgmt_file.test
   @@ -19,11 +19,185 @@ class TMGMTFileTestCase extends TMGMTBaseTestCase {
   +    $job->addItem('test_source', 'test_html', '1');
   ...
   +    // Note that here we use only simple HTML structure so that we can do the
   +    // source and translation comparison. If more complex content including also
   +    // html entities or other non ASCII chars is used the comparison might not
   +    // work due to XML processors manipulating with such chars.
   

   Other tests that require special data usually just override the default, that is IMHO easier to read than an implicit thing through the type. But we're doing it elsewere too, so not sure. Doing it here would make it easier to test multiple different structures. And I know it contradicts with what I'm saying below (setting data directly being problematic), but it's a test here :))

5. +++ b/translators/file/tmgmt_file.test
   @@ -19,11 +19,185 @@ class TMGMTFileTestCase extends TMGMTBaseTestCase {
   +    $job->requestTranslation();
   +    // Make sure the masking process happened.
   +    $text = $this->findSourceText($job->getData());
   +    $this->assertTrue(strpos($text, '<bpt') !== FALSE);
   +    $this->assertTrue(strpos($text, '<ept') !== FALSE);
   

   Testing a bit more specific would make sense I think, e.g a whole string from opening to closing tag, not just a small bit of it.

6. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
   @@ -0,0 +1,381 @@
   +        if (isset($job_items[$tjiid])) {
   +          $label = $job_items[$tjiid]->label();
   +        }
   +        else {
   +          $label = t('unknown');
   +        }
   +        throw new TMGMTException(t('Integrity check for job item @label failed.', array('@label' => $label)));
   

   How could it end in the else case?

7. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
   @@ -0,0 +1,381 @@
   +          if ($reader->getAttribute('ctype') == 'image') {
   +            $text .= '<img';
   +            while ($reader->moveToNextAttribute()) {
   +              // @todo - we have to use x-html: prefixes for attributes.
   +              if ($reader->name != 'ctype' && $reader->name != 'id') {
   +                $text .= " {$reader->name}=\"{$reader->value}\"";
   +              }
   +            }
   

   Do we need to worry about escaping/encoding here?

8. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
   @@ -0,0 +1,381 @@
   +
   +  /**
   +   * Helper function to process the source text.
   +   *
   +   * @param array $data
   +   *   Job data array.
   +   */
   +  protected function processSource(&$data) {
   

   "Helper function to" is IMHO equally useless as "Will", "protected function" tells me that as well, what about "Masks HTML tags" or someting like that, that actually describes what it does?

   Also, either the two loop functions should have a default implementation with abstract functions for this processSource() thing, or we give it a better name, not "process".

9. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
   @@ -0,0 +1,381 @@
   +      $this->elementsCount = 0;
   

   Are you sure this is the right place to set elementsCount? Won't this set it back to 0 for every data item?

   Also, the trick with $this->elementsCount that's then used in the parent function seems... rather weird, I'd either return it (a bit more code but I think easier to understand) or pass down the job so that it can be immediately updated. The second is what I'd do if we have a default implementation for that loop.

10. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
    @@ -0,0 +1,381 @@
    +      $dit = new RecursiveIteratorIterator(
    +        new RecursiveDOMIterator($dom),
    +        RecursiveIteratorIterator::SELF_FIRST);
    

    Variable names are usually not shortened, can we name this $iterator? Or $dom_iterator?

11. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
    @@ -0,0 +1,381 @@
    +
    +        if ($node->nodeType === XML_ELEMENT_NODE) {
    +          // We have a new tag, increment the id.
    +          $id++;
    +          // Currently the incremental id and the elementCount has the same
    

    id in comments should be ID

12. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
    @@ -0,0 +1,381 @@
    +      }
    ...
    +      foreach ($node->attributes as $attribute) {
    +        $attributes[] = $attribute->name . '="' . $attribute->value . '"';
    ...
    +    $beginning_tag = '<' . $node->nodeName;
    +    if ($node->hasAttributes()) {
    +      $attributes = array();
    +      /** @var DOMAttr $attribute */
    ...
    +
    +      $beginning_tag .= ' '. implode(' ', $attributes);
    +    }
    +    $beginning_tag .= '>';
    +    $writer->startElement('bpt');
    

    Same here, do we need to handle escaping of attribute values? AFAIK, we do parse this again when we import it, no?

13. +++ b/translators/file/tmgmt_file.text_processor.xliff_mask_html.inc
    @@ -0,0 +1,381 @@
    +    $writer->endElement();
    ...
    +    $writer->writeAttribute('ctype', 'image');
    +    foreach ($node->attributes as $attribute) {
    +      // @todo - uncomment when issue with Trados/sub elements fixed.
    +//      if (in_array($attribute->name, array('title', 'alt'))) {
    +//        continue;
    +//      }
    +      $writer->writeAttribute($attribute->name, $attribute->value);
    +    }
    +//    if ($alt_attribute = $node->getAttribute('alt')) {
    +//      $writer->startElement('sub');
    +//      $writer->writeAttribute('id', $id . '-img-alt');
    +//      $writer->writeAttribute('ctype', 'x-img-alt');
    +//      $writer->text($alt_attribute);
    +//      $writer->endElement();
    +//      $this->elementsCount++;
    +//    }
    +//    if ($title_attribute = $node->getAttribute('title')) {
    +//      $writer->startElement('sub');
    +//      $writer->writeAttribute('id', $id . '-img-title');
    +//      $writer->writeAttribute('ctype', 'x-img-title');
    +//      $writer->text($title_attribute);
    +//      $writer->endElement();
    +//      $this->elementsCount++;
    +//    }
    

    Comment characters need to be indendeted correctly as well, if this is will be committed like this which I understand it will.

14. +++ b/translators/file/tmgmt_file.ui.inc
    @@ -44,6 +44,15 @@ class TMGMTFileTranslatorUIController extends TMGMTDefaultTranslatorUIController
     
    +    $text_processors = $translator->getSetting('text_processor_plugins');
    +    $form['text_processor_plugins'] = array(
    +      '#type' => 'checkboxes',
    +      '#title' => t('Text processors'),
    +      '#description' => t('Identify which text processors should be used during the import/export operations.'),
    +      '#options' => tmgmt_file_text_processor_plugin_labels(),
    +      '#default_value' => is_array($text_processors) ? $text_processors : array(),
    

    Ok, so this is a setting on the translator, so it can't be configured per job but you can configure the format per job...

That's it for now with specific feedback, going to discuss the whole thing with Miro asap.

Here is patch that deals with characters that break xml structure. Have not yet done anything from reviews.

Ok, to summarize, here is my main problem with the way the plugin system works right now: There's a disconnect between the actual implementation, the way it's called and how it works:

- It works on the job, giving the assumption that it's a generic thing, this also makes the implementations rather complicated (that foreach, which *could* be solved with a base implementation). With the exception of the validate thing, which receives a file name (without knowing anything about it)
- It's called from the file translator, no matter what's configured
- The actual implementation however blindly assumes that the source is HTML and the target will be XLIFF.

The result is that it's a one-off solution for this specific problem, masked as a more or less generic pluggable system.

I think there are 3 options as to what we could do:
1 Write a fancy and complex processing system, that would actually be generic. That would very likely involve introducing a intermediate markup language that the source is converted into and then the translator processes it back into whatever he needs. That would probably keep us busy for a year and is overkill for this issue :)
2. Change the plugin system so that it's specific to xliff, only execute those plugins if xliff is selected and update documentation/possibly methods.
3. Implement this as an actual one-off solution, we can keep the class but just call it directly.

I actual prefer 3, because 2 will just get in the way in case we actually implement something like 1 at some point and 3 would then be easier to incorporate into that new plugin system as it wouldn't be a public API before. We can also make arguments/methods match that specific use case and don't have to think what we need to do to support every possible use case.

Then there's the the problem with saving the data in those methods in the class. That is problematic, because sooner or later, we will introduce things like checking if the source still matches when you review a translation and at least warn if not. That will happen unless we apply the first part of the processing *before* we do that, which we can't unless we'd implement 1. Given that we don't need the fixed source as we have the tag count it's supposed to have (afaik), would it be easier to just run $data through the processor, without persisting it?

Did not implement all the comments yet. Focused on your 3. and implemented it. I agree with you here, but the original specification said that it should be pluginable and that there should be an option to turn off masking.

Re that setting - i think it does not have a sense as that setting would be about "Export xliff" and "Export even more xliffed file".

Tested with Trados studio, works fine.

Please see the patch if you like the conceptual changes and will finish other comments later on.

here is the final thing

- Implemented comments
- Ensured element ids are unique in the whole xliff document

make ids "nicer"

- Here comes the setting to enable/disable xliff processing
- HTML semantics integrity check that triggers only error messages but the job gets imported

36: 1882108-mask_html-12.patch queued for re-testing.

1. +++ b/translators/file/tmgmt_file.format.xliff.inc
   @@ -347,12 +346,14 @@ class TMGMTFileformatXLIFF extends XMLWriter implements TMGMTFileFormatInterface
   @@ -371,7 +372,11 @@ class TMGMTFileformatXLIFF extends XMLWriter implements TMGMTFileFormatInterface
   
   @@ -371,7 +372,11 @@ class TMGMTFileformatXLIFF extends XMLWriter implements TMGMTFileFormatInterface
    
        $tray = array();
        $non_pair_tags = array('br', 'img');
   -    $elements_count = 0;
   +
   +    if (!isset($this->job->settings['xliff_validation'])) {
   +      $this->job->settings['xliff_validation'] = array();
   +    }
   +    $xliff_validation = &$this->job->settings['xliff_validation'];
   

   Can you explain why doing this with a by-ref assignment, instead of setting it again at the end? This would be more complicated to port to 8.x (where we will likely have more methods to work with properties/fields) and you don't really save code as the explanation is as long as the code would be :)

2. +++ b/translators/file/tmgmt_file.format.xliff.inc
   @@ -432,8 +440,8 @@ class TMGMTFileformatXLIFF extends XMLWriter implements TMGMTFileFormatInterface
    
   -    // Set the elements count for the current item into job settings.
   -    $this->job->settings['elements_counts'][$tjiid] = $elements_count;
   +    // Set the xliff_validation data that were set into the
   +    // $this->job->settings['xliff_validation'] array().
        $this->job->save();
   

   The comment also doesn't make too much sense right now, as we don't set anything, we just save it. array() should also be just array.

3. +++ b/translators/file/tmgmt_file.test
   @@ -120,31 +120,25 @@ class TMGMTFileTestCase extends TMGMTBaseTestCase {
   -      if ($message->getMessage() == t('Failed to validate file, import aborted.')) {
   +      debug($message->getMessage());
   +      if ($message->getMessage() == t('Failed to validate semantic integrity of %key element. Please check also the HTML code of the element in the review process.', array('%key' => '2][dummy][deep_nesting'))) {
   

   Left-over debug.

fixed

this is already implemented

reroll

here we go

Ok, committed and pushed!

Given that this is a pretty major change, let's document this change as a change notice: https://drupal.org/list-changes/tmgmt

Sorry, but I have a very important question and I can't find an answer in the
documentation yet: I get the result back from the translator as escaped HTML
and import it, but it doesn't seem to be converted back to HTML at any point
of time. How can I solve that? What am I doing wrong? :(

Hello miro and berdir

I have the exact same problem like "Anybody" ;) In the xlif is htmlentites-encoded markup and i want this to converted back to the original HTML-Markup on import. Can you give me a hint how this can be achieved ?

many thanks for your answer
Jan

P.S. Here is an example:

<trans-unit id="53][field_text][0][value" resname="53][field_text][0][value">
	<source xml:lang="fr">&lt;strong&gt;This is text in france&lt;/strong&gt;</source>
	<target xml:lang="nl">&lt;strong&gt;This is text in dutch&lt;/strong&gt;</target>
	<note>Answer</note>
</trans-unit>

Any progress or even an idea here?

Here's a quick fix for new translation imports for 7.x1.0-rc1.

Add this to entity/tmgmt.entity.job_item.inc on line 630 or if that isn't right in function addTranslatedDataRecursive near the end before $this->updateData:

$values['#translation']['#text'] = str_replace('&lt;', '<', $values['#translation']['#text']);
$values['#translation']['#text'] = str_replace('&gt;', '>', $values['#translation']['#text']);

Again, quick fix so use at your own risk.

Thanks a lot @dragonfire535 for your quickfix.
The problem with it is that there are several further HTML entities that should be decoded like &nbsp; and other ascii characters that may be contained in the text.

I've created and attached a patch that uses html_entity_decode() to do that in the last step. You can use it as quick fix.

Anyway I think we should discuss
to add a setting or permission to enable this HTML encoding
add a function to handle the text securely, at least add filter_xss_admin (see patch) or custom filter_xss (See: https://www.drupal.org/node/28984)
Further security implications because code injection will definitely be possible if we're not careful here.

Anyway this has to be fixed because otherwise a proper HTML import is not possible otherwise.

Could a tmgmt maintainer and/or a member of the Drupal security team have a look at this perhaps finally?

And finally for clarification: You need the patch from #42 plus this one to have the full export and import functionality for HTML translations!

Removed unrelated code style fixes from the patch.

@Anybody, thank you for the path #55. It was a good hint for me in the Drupal 8 version as well.
I vote +1 for RTBC.

Thanks for the patch! It is working for us but a couple nitpicks below. Cheers.

1. +++ b/entity/tmgmt.entity.job_item.inc
   @@ -627,6 +627,10 @@ class TMGMTJobItem extends Entity {
   +        if(!empty($values['#translation']['#text'])) {
   

   Need a space after if.

2. +++ b/entity/tmgmt.entity.job_item.inc
   @@ -627,6 +627,10 @@ class TMGMTJobItem extends Entity {
   +          // If the text contains HTML entities, decode them to have proper HTML back in the content.
   

   Comment should wrap at 80 characters (right after HTML without leaving a dangling space).