Add methods in JobItem to get segmented data

1. +++ b/src/Entity/JobItem.php
   @@ -386,6 +386,25 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
   +  public function getSegmentedData($key = array(), $index = NULL) {
   
   @@ -396,6 +415,26 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
   +  public function getSegmentedSourceData() {
   

   Both implementations look very similar. Why would we want to have two new methods?
   IMHO the goal is that the source is always segmented when we add data to a job. Accessing the data thus doesn't need triggering any segmenter again.
   (For past jobs we can't change the situation anyway.)

2. +++ b/src/Entity/JobItem.php
   @@ -396,6 +415,26 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
   +        $unfiltered_data = $plugin->getData($this);
   ...
   +    return $segmented_data;
   

   Hm, getSegmentedSourceData should IMHO return unsegmented data if the segmenter is not installed. Currently it returns pure emptyness...

I start to understand parts here. The problem is that the calls are not provided and the interfaces are not described in how they should be used.

The segmenter could be transparent.

If it only detects a single segment, it doesn't need to annotate a segment at all.
IMHO even a lengthy title could contain multiple linguistic segments.
But we could also define that we only trigger the segmenter for format aware fields.

Also, the issue should also call the segmenter and make TMGMT use the segmenter.. (uh, test dependencies...)
Including test coverage. Otherwise we are stepping in the dark.
We can't commit the methods as is.

The segmenter should not only return the string. It should also be able to attach OO data to each element.
Thus, we can later reuse the parsed data without triggering the parser again.

OK, more thoughts:

Returning data with an additional key containing segment information is a good idea for API consistency.
We can still offer some helpers to work with it, if it simplifies code.

To run a test successfully, we can provide a dummy segmenter service in the test. Make it return some variable like a mock does, so the calling text can set what the service should return.

But what we learn from this is, that the segmenter interface needs to be introduced by tmgmt, while the tmgmt_memory can only implement it. Otherwise interface annotations and decoupling doesn't work.

Hope this helps a bit to move forward and bring the segmentation back to tmgmt.

+++ b/src/Entity/JobItem.php
@@ -1129,4 +1131,29 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
+          $field['#segmented_data'] = $segmenter->segmentData($field['#text']);
...
+            $field['#translation']['#segmented_data'] = $segmenter->segmentData($field['#translation']['#text']);

Ohh, those are different things.

Mind that on the source side, a segment originates. When the source is parsed for the first time, segments are identified.

On the translation side, the segmenter is only allowed to detect segments that are passed into translation and exist on the source side. We should only detect segments on the right side if they have a pair.
By blindly segmenting the right side, you could detect different segments and that will create huge trouble..

Probably just stating the obvious, but so far, here's what I've got :

The test fails at at line 227 of TMGMTUITest.

Line 219 : $this->drupalGet('admin/tmgmt/items/' . $item4->id());.

Going to the newly created job item (line 216) and saving it (line 225). tjiid here is 6. Job 3 has job items 5 and 6.

This redirects to the job overview page, where the job items are listed.

Line 226 : $this->clickLink('View');.

In a successful run, this clicks on job item 5. With the patch, this clicks on job item 6. So, job item 5 was last modified in a successful run and job item 6 was last modified with the new patch. What I'm not sure to understand is why this code worked in the first place, since job item 6 is created and saved after job item 5 in both cases.

As discussed with @edurenye, fixing tests seems most appropriate here. Other than that, I did not investigate much to see if the data segmentation was working fine :-/.

I'm trying to document some thoughts about usage of the Segmenter.

1. +++ b/src/Entity/JobItem.php
   @@ -1091,4 +1100,30 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
   +      foreach ($fields as $key => $field) {
   +        if (isset($field['#translate'])) {
   +          if (!isset($field['#segmented_data'])) {
   +            $field['#segmented_data'] = $segmenter->segmentData($field['#text']);
   +          }
   +          else if (isset($field['#translation'])) {
   +            $field['#translation']['#segmented_data'] = $segmenter->segmentData($field['#translation']['#text']);
   +          }
   

   Now the idea of #segmented_data is tightly coupled between tmgmt, tmgmt_memory and the segmenter.
   Why are we not passing in the items completely to the segmenter and it has access to it and can attach #segmented_data on its own?

   +++ b/src/Entity/JobItem.php
   @@ -1091,4 +1100,30 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
        foreach ($fields as $key => $field) {
          if (isset($field['#translate'])) {
            $segmenter->segmentData($field);
          }
   

   The segmenter then would do the checks above.
   That's much more flexibility inside the segmenter to improve the situation.
   It would be the internal choice of the segmenter to attach #segmented_data to the $field.
   The key here is that $field is passed by reference.

2. +++ b/src/Form/JobItemForm.php
   @@ -294,10 +294,25 @@ class JobItemForm extends TmgmtFormBase {
   +        $segmenter = \Drupal::service('tmgmt.segmenter');
   +        $array_key = explode('|', $key);
   +        $translated_segmented_data = $segmenter->segmentData($label);
   +        $source_data = $this->getEntity()->getData($array_key);
   +        if (isset($source_data['#segmented_data'])) {
   +          $source_segmented_data = $source_data['#segmented_data'];
   +          $source_segments = $segmenter->getSegmentsOfData($source_segmented_data);
   +          $translated_segments = $segmenter->getSegmentsOfData($translated_segmented_data);
   +          if (count($source_segments) != count($translated_segments)) {
   +            $form_state->setError($form['review'][$group_key][$parent_key][$key]['translation'], $this->t('The translation has different amount of segments than the source text.'));
   +          }
   +        }
   

   This would translate to:

          $segmenter = \Drupal::service('tmgmt.segmenter');
          $array_key = explode('|', $key);
          $source_data = $this->getEntity()->getData($array_key);
          $element =& $form['review'][$group_key][$parent_key][$key]['translation'];
          $segmenter->validateSegments($source_data, $element, $form_state);
   

   Whereas again $source_data here is then internally checked for #segmented_data and source and translation segments.
   Also the segmenter can independently check health (can also improve to much more than just counts) and display proper errors.

   This would make the dependency much more reduced and allow more freedom to the Segmenter.
   Also TMGMT has less assumptions about how the Segmenter internally works IMHO.

   Still not sure and open for discussions to improve or learn i was wrong. :-)

3. +++ b/src/Form/JobItemForm.php
   @@ -294,10 +294,25 @@ class JobItemForm extends TmgmtFormBase {
   +        $translated_segmented_data = $segmenter->segmentData($label);
   

   Hm, why getting the segmentData from $label?

And here is the review finally.

Spoiler: You will not like it :)

1. +++ b/src/Entity/JobItem.php
   @@ -368,10 +368,12 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
        if (empty($this->unserializedData) && $this->get('data')->value) {
          $this->unserializedData = unserialize($this->get('data')->value);
   +      $this->addSegmentedData();
        }
        if (empty($this->unserializedData) && $this->getJobId()) {
          // Load the data from the source if it has not been set yet.
          $this->unserializedData = $this->getSourceData();
   +      $this->addSegmentedData();
   

   the first call to segmenting will do it and persist it. The source text will not change anymore, there is no need to repeat this process every time we unserialize*.

   So instead, we can move this call into getSourceData() and only need to call it from a single place.

   * One exception: We have to possibility to read the source data again if it changed on review. We need a follow-up to figure out what needs to happen there. Basically, we will need to explicitly throw away the segmented data and do it again. But that can come later and is not important for step one.

2. +++ b/src/Entity/JobItem.php
   @@ -782,6 +784,13 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
            NestedArray::setValue($this->unserializedData, array_merge(\Drupal::service('tmgmt.data')->ensureArrayKey($key), array($index)), $value);
   +        if ($index == '#text' && \Drupal::hasService('tmgmt.segmenter')) {
   

   Instead of checking for the existence of a service that is actually in our namespace, we register an empty implementation in tmgmt. That will not do anything and not return anything but it makes the code easier to read.

   memory might then need to implement a ServiceProvider and replace the class (might still work to just define it if it follows later in the list of modules but that is a bit unreliable).

3. +++ b/src/Entity/JobItem.php
   @@ -782,6 +784,13 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
            NestedArray::setValue($this->unserializedData, array_merge(\Drupal::service('tmgmt.data')->ensureArrayKey($key), array($index)), $value);
   +        if ($index == '#text' && \Drupal::hasService('tmgmt.segmenter')) {
   +          /** @var \Drupal\tmgmt\SegmenterInterface $segmenter */
   +          $segmenter = \Drupal::service('tmgmt.segmenter');
   +          $segments = $segmenter->segmentData($value);
   +          NestedArray::setValue($this->unserializedData, array_merge(\Drupal::service('tmgmt.data')
   +            ->ensureArrayKey($key), array('#segmented_data')), $segments);
   +        }
   

   Need to check when this scenario happens exactly. Could be for the update case I mentioned above, but it could also be when a translation is written back. that would not be correct, see below. So this might need to go away here...

4. +++ b/src/Entity/JobItem.php
   @@ -1091,4 +1100,30 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
   +          if (!isset($field['#segmented_data'])) {
   +            $field['#segmented_data'] = $segmenter->segmentData($field['#text']);
   +          }
   +          else if (isset($field['#translation'])) {
   +            $field['#translation']['#segmented_data'] = $segmenter->segmentData($field['#translation']['#text']);
   +          }
   +          NestedArray::setValue($this->unserializedData, \Drupal::service('tmgmt.data')
   +            ->ensureArrayKey($key), $field);
   

   I'm fine with miros suggestion to give the segmenter the whole array (Which btw needs to be called $data_items/$data_item, *not* $fields)

   However, we must never segment the translated text. Our only hope is that the translator is nice and respects and supports the segmented data. If not, there is absolutely nothing that we can do.

   As an example (if you don't believe me), take a right-to-left language like russian.

   Source text:
   Sentence A. Sentence B. Sentence C.

   The translation that will come back will basically look like this:
   .C ecnetneS .B ecnetneS .A ecnetneS

   Your implementation would assign "Sentence A" to "C ecnetneS".

   Which clearly is wrong :)

   Yes, that means that every translator that we want to support translation memory with needs to be updated to somehow work with segmented data instead of the current text.

   We can make some assumptions, like if there's only one source segment and at some point, reviewers might have a tool (hi Sasa) to fix segments if they got lost/broken but we're a few steps away from that.

   Yes, to actually test your memory, it means you need to have a translator that supports this. Doing that with test translator should be do-able. You could also patch google/microsoft to send out the segmented string and hope that they're nice to our segment tags.

   We'll also see there we might need to do something with segmented data, but more likely the opposite of this (removing segments and put into #text when only #segmented_data is set (which I would btw name #segmented_text).

5. +++ b/src/Form/JobItemForm.php
   @@ -294,10 +294,25 @@ class JobItemForm extends TmgmtFormBase {
   +        $translated_segmented_data = $segmenter->segmentData($label);
   

   Nice variable name ;)

6. +++ b/src/SegmenterInterface.php
   @@ -0,0 +1,51 @@
   +/**
   + * This Service interface offers a suite of methods to add segments and filter data.
   + */
   +interface SegmenterInterface {
   

   There isn't really such a thing as "Service interface".

   I'd just simplify this to "Offers a ...". solves the >80 chars problem too.

1. +++ b/src/Entity/JobItem.php
   @@ -386,7 +384,11 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
   +      $segmenter = \Drupal::service('tmgmt.segmenter');
   +      $segmented_data = $segmenter->getSegmentedData($data);
   +      return $segmented_data ? $segmented_data : $data;
   

   could be simplifed to return $segmenter->getSegmentedData() ?: $data;. Or we could even say it must return $data, then that wouldn't be necessary.

2. +++ b/src/Entity/JobItem.php
   @@ -386,7 +384,11 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
      }
   
   +++ b/src/Form/JobItemForm.php
   @@ -278,28 +278,30 @@ class JobItemForm extends TmgmtFormBase {
          if (isset($value['data']['translation']['value'])) {
   -        $label = $value['data']['translation']['value']['#value'];
   +        $data_item = $value['data']['translation']['value']['#value'];
          }
   

   that's also not really correct, $data item means an array with #text, #translation and so on. This is really just the text, so $translation_text or so.

3. +++ b/src/Form/JobItemForm.php
   @@ -278,28 +278,30 @@ class JobItemForm extends TmgmtFormBase {
   +      $segmenter = \Drupal::service('tmgmt.segmenter');
   +      $translated_segmented_data = $segmenter->segmentData($data_item);
   +      if ($translated_segmented_data) {
   

   But this means you're still trying to segment the translation text? As explained above, I don't think that is possible at all.

   Yes, without this, it's impossible to use tmgmt_memory without Sasa's project. But I don't see another way.

   You could try to just show the segments and hope they don't get broken.

   See also #2757813: Provide support for masking HTML tags while editing, the idea there is that be able to switch out the text that we show, what that means is that we could show the segment tags if memory is enabled, at least as a Proof of Concept.

4. +++ b/src/SegmenterBase.php
   @@ -0,0 +1,30 @@
   +/**
   + * This Service offers a base implementation for the SegmenterInterface.
   + */
   +class SegmenterBase implements SegmenterInterface {
   

   Base classes are usually abstract and have actual logic. Without any code, there is no benefit from extending this class.

   I'd rather name this NullSegmenter or something like that.

1. +++ b/src/Form/JobItemForm.php
   @@ -278,21 +278,38 @@ class JobItemForm extends TmgmtFormBase {
   -      else {
   -        drupal_set_message(t('Validation completed successfully.'));
   +      /** @var \Drupal\tmgmt\SegmenterInterface $segmenter */
   +      $segmenter = \Drupal::service('tmgmt.segmenter');
   +      $translated_segmented_data = $segmenter->segmentData($translation_text);
   +      if ($translated_segmented_data) {
   +        /** @var \Drupal\tmgmt\SegmenterInterface $segmenter */
   +        $segmenter = \Drupal::service('tmgmt.segmenter');
   +        $array_key = explode('|', $key);
   +        $data = $this->getEntity()->getData($array_key);
   +        if (isset($data['#segmented_text'])) {
   +          $segmented_data = $data['#segmented_text'];
   +          $segments = $segmenter->getSegmentsOfData($segmented_data);
   +          $translated_segments = $segmenter->getSegmentsOfData($translated_segmented_data);
   +          if (count($segments) != count($translated_segments)) {
   ...
   +          }
   

   Ok. Discussed this part with Miro.

   Agree with what he is saying in his earlier review. All of this should be a method on the segmenter, something like validateFormTranslation() or something like that. That receives argument like the form element ($form['review'][$group_key][$parent_key][$key]), the array_key, $form_state and whatever else you need.

   Then you can experiment with validations in your module, and the segmenter can do validations based on what he supports and understands. Count is just one example. We might also want to validate if the segment hashes are the same, for example.

2. +++ b/src/SegmenterInterface.php
   @@ -0,0 +1,62 @@
   + * This interface offers a suite of methods to add segments and filter data.
   + */
   +interface SegmenterInterface {
   

   Lets mark this interface as @internal to clarify that it is not yet a public/stable API and will change in the future.

3. +++ b/src/SegmenterInterface.php
   @@ -0,0 +1,62 @@
   +   * Add the "tmgmt-segment" tags to each paragraph or division/section.
   +   *
   +   * This function will return the same data adding the "tmgmt-segment"
   +   * XML Element with an attribute "id" that will identify the segment.
   +   * This XML Element will be added as the parent element of each paragraph
   +   * "<p>" or division/section "<div>".
   +   *
   +   * @param string $data
   +   *   A string with the XML serialized data.
   +   *
   +   * @return string
   +   *   A string with the data with the tags added.
   +   */
   +  public function segmentData($data);
   

   This method will then no longer be on the public inteface.

4. +++ b/src/SegmenterInterface.php
   @@ -0,0 +1,62 @@
   +  public function filterData($data);
   ...
   +  public function getSegmentsOfData($data);
   ...
   +  public function getSegmentedData($data);
   

   Those three methods need better documentation, some things are simply wrong (getSegmentedData() does *not* receive a string). They should also have better variable names (if it is a string, do not use $data. Use $text, $segmented_text, ..).

   Where useful, examples of the expected/returned data structures should also be included.

   One also mentions segments delta, should be id.

+++ b/src/SegmenterBase.php
@@ -0,0 +1,30 @@
+/**
+ * This Service offers a base implementation for the SegmenterInterface.
+ */
+class SegmenterBase implements SegmenterInterface {

Base classes are usually abstract and have actual logic. Without any code, there is no benefit from extending this class.

I'd rather name this NullSegmenter or something like that.

Don't forget to update it also in other modules where it is used, like in Drupal\tmgmt_memory\Segmenter ;)

Very close now ;)

+++ b/src/SegmenterInterface.php
@@ -1,62 +1,89 @@
+   * If the key '#translation' exists it will segment it as well.
...
+   *       '#segmented_text' => '<tmgmt-segment>Segmented target text</tmgmt-segment>',

remove those lines, because while you might still do it, I'm still convinced it is wrong (and I think we're slowly convincing you as well).

Yeah this starts to look awesome! Some nits...

1. +++ b/src/Entity/JobItem.php
   @@ -384,7 +384,10 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
   +      return $segmenter->getSegmentedData($data) ?: $data;
   

   Is the fallback to $data needed? I think not. The segmenter should always return data.

2. +++ b/src/NullSegmenter.php
   @@ -0,0 +1,32 @@
   +class NullSegmenter implements SegmenterInterface {
   

   The NullSegmenter should still consider ALL valid return values!

3. +++ b/src/NullSegmenter.php
   @@ -0,0 +1,32 @@
   +  public function getSegmentedData($data) {}
   

   return $data;

+++ b/src/NullSegmenter.php
@@ -12,17 +12,23 @@ class NullSegmenter implements SegmenterInterface {
    */
-  public function filterData($data) {}
+  public function filterData($segmented_text) {
+    return NULL;
+  }
 

this should return $segmented_data?

+++ b/tests/src/Kernel/TestSegmenterTest.php
@@ -0,0 +1,117 @@
+ */
+class TestSegmenterTest extends TMGMTKernelTestBase {

We should also add a test to test the test segmenter test.

More seriously, most of this is pointless :) We're testing hardcoded dummy implementations in a test implementation.

You use test implementations to test the other part of your code (like that the method is being called), you don't test them directly. See below.

+++ b/tests/src/Kernel/TestSegmenterTest.php
@@ -0,0 +1,117 @@
+  public function testFilterData() {
+    $segmented_sample = file_get_contents(drupal_get_path('module', 'tmgmt') . '/tests/testing_html/segmented_sample.html');
+    $filtered_sample = file_get_contents(drupal_get_path('module', 'tmgmt') . '/tests/testing_html/sample.html');
+    /** @var \Drupal\tmgmt\SegmenterInterface $segmenter */
+    $segmenter = \Drupal::service('tmgmt.segmenter');
+
+    $filtered_data = $segmenter->filterData($segmented_sample);
+    $this->assertEquals(str_replace(PHP_EOL, '', $filtered_sample), $filtered_data);
+  }

This one for example. You load test data, and ensure that you get hardcoded test data back. But the test implementation doesn't even look at $data.

Only provide a test implementation if you want to test something else remove this test and the implementation for it. It is not needed.

Same for testSegmentData(), testGetSegmentsData(), and that's all of this class ;)

Yayy, committed. :-)

Doesn't look that complex anymore. And most importantly all known cases can be covered in pluggability now.
Looking forward to see real experimental implementations and learn from its usage.