Add line numbers to Split Fields

+++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
@@ -142,13 +142,54 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
+            'class' => [isset($field_diff_rows[$key][0]['class']) ? $field_diff_rows[$key][0]['class'] : NULL, $field_diff_rows[$key][1]['class']]

You should add comma after last element in array. https://www.drupal.org/docs/develop/standards/coding-standards#array

+++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
@@ -142,13 +142,54 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
+            'class' => [isset($field_diff_rows[$key][2]['class']) ? $field_diff_rows[$key][2]['class'] : NULL, $field_diff_rows[$key][3]['class']]

Here the same.

+++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
@@ -142,13 +142,54 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
+

You have one extra line here.

Providing screenshot for patch-3

+++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
@@ -142,13 +142,54 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
+      //CUSTOM
...
+      //END OF CUSTOM

Please remove this.

Besides that is better to test manually too.

There are conditional statements and I tested it as it is suggested in comment #9.
I also removed custom code comments as suggested in comment #8.
There is a screenshot provided.

I have provided another example which can pull maybe potential issues.
This example is created with using "enter" instead of "shift+enter" and creating whitelines between the rows. When the whiteline is removed, layout considers it as a change. Now all of the lines that have data have line numbers, like in Unified fields layout.

+++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
@@ -142,13 +142,56 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
+        if(trim($field_diff_rows[$key][3]['data']['#markup']) != "") {

Space between "if" and "(".

+++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
@@ -142,13 +142,56 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
+        if(trim($field_diff_rows[$key][3]['data']['#markup']) != "") {

Here the same.

Rebased

Test changes that should follow the changes of the Split fields layout. Together with changes from the patch https://www.drupal.org/node/2800155#comment-11659699 to remove additional errors "Illegal string offset".

Reversed changes from https://www.drupal.org/node/2800155#comment-11659699 issue that were applied by mistake.
Changes made to the Split fields to evade "Illegal string offset errors"

Fixed "Illegal string offset errors" completely

+++ b/src/Controller/PluginRevisionController.php
@@ -281,7 +281,7 @@ class PluginRevisionController extends ControllerBase {
-
+  ¶

unrelated change. :)

Feature added, numbers are removed together with columns on "strip_tags".
I am working on test fixes.

+++ b/src/Tests/DiffRevisionTest.php
@@ -121,10 +121,8 @@ class DiffRevisionTest extends DiffTestBase {
-    $this->assertEqual((string) (($diff_row[1]->span)), '1');
...
-    $this->assertEqual((string) (($diff_row[3]->span)), '2');

We need these lines. :)

+++ b/src/DiffLayoutManager.php
@@ -87,7 +87,7 @@ class DiffLayoutManager extends DefaultPluginManager {
-   *
+   *git sta

unrelated change. :)

+++ b/src/Tests/DiffRevisionTest.php
@@ -121,10 +121,8 @@ class DiffRevisionTest extends DiffTestBase {
-    $this->assertEqual((string) (($diff_row[1]->span)), '1');
...
-    $this->assertEqual((string) (($diff_row[2])), '+');
-    $this->assertEqual((string) (($diff_row[3]->span)), '2');

@@ -240,12 +238,12 @@ class DiffRevisionTest extends DiffTestBase {
-    $this->assertEqual(htmlspecialchars_decode(strip_tags($diff_row[3]->asXML())), 'new body');
...
-    $this->assertEqual(htmlspecialchars_decode(strip_tags($diff_row[3]->asXML())), 'newer body');

This lines should remain the same. :)

I was taking over this, forgot to assign the issue to myself and have seen later that a patch has been already uploaded...

Then I'll post some feedback/considerations:

1. +++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
   @@ -142,13 +142,67 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
   +        if(isset($field_diff_rows[$key][1]['data']) && trim($field_diff_rows[$key][1]['data']['#markup']) != '') {
   ...
   +        if(isset($field_diff_rows[$key][3]['data']) && trim($field_diff_rows[$key][3]['data']['#markup']) != '') {
   

   I suggest you to change the settings in phpStorm to avoid this if(.

2. +++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
   @@ -142,13 +142,67 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
   +            'class' => ['diff-line-number', isset($field_diff_rows[$key][1]['data']) ? $field_diff_rows[$key][1]['class'] : NULL],
   

   Compared with the code in UnifiedFieldsDiffLayout, is this kind of check (and more in this code) really needed, or is the code in the other file not safe and need to be improved?

3. +++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
   @@ -142,13 +142,67 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
   +        if ($active_filter == 'strip_tags') {
   ...
   +    if ($active_filter == 'strip_tags') {
   

   To make it more similar to the other code, you might need to add $raw_active = $active_filter == 'raw'; before the $fields foreach loop and replace $active_filter == 'strip_tags' with !$raw_active.

4. +++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
   @@ -142,13 +142,67 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
   +          $final_diff[$row_counter] = [$final_diff[$row_counter][1],
   +            $final_diff[$row_counter][2], $final_diff[$row_counter][4],
   +            $final_diff[$row_counter][5]];
   

   Format this better, like:

   $array = [
       $item1,
       $item2,
       $item3,
   ];

5. +++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
   @@ -142,13 +142,67 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
   +      }
          // Add the field label to the table only if there are changes to that field.
   

   Add a new line here to separate the code (readability).

6. +++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
   @@ -181,11 +235,11 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
   +      'colspan' => 4,
   

   Is it allowed to set this (as 3) differently than the second colspan or must they have the same value ?

Otherwise UI looks good!

About 41.2 yes, those checks are needed otherwise we will get some php warnings.

Applied changes from comments 38-41. Check in the 41. 2) is to avoid the class offset errors in tests.

+++ b/src/Tests/DiffRevisionTest.php
@@ -97,12 +97,12 @@ class DiffRevisionTest extends DiffTestBase {
-    $this->assertEqual((string) (($diff_row[1]->span)), '1');
...
-    $this->assertEqual((string) (($diff_row[3]->span)), '2');

Almost! We don't want to remove this. :)

Looks really nice now. :)

Some last nitpicks:

1. +++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
   @@ -115,12 +115,13 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
   +      $raw_active = $active_filter == 'raw';
   

   This should be defined outside foreach ($fields as $field).

   The code in the other file needs improvement as well, need a followup to improve/cleanup that code.

2. +++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
   @@ -142,13 +143,70 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
   +        ];
   +        if (!$raw_active) {
   

   New line (readability).

3. +++ b/src/Plugin/diff/Layout/SplitFieldsDiffLayout.php
   @@ -142,13 +143,70 @@ class SplitFieldsDiffLayout extends DiffLayoutBase {
   +            $final_diff[$row_counter][5]];
   

   Add a come after the last element and ]; in a new line (coding standard).

Then is fine :)

Even better now.

Agree

Oh wait, missed 41.6)
Mind the difference between the following (REV2 header):

Just a small thing, otherwise is ok.

True, the colspan should be 3 and not 4. We also need new screenshots after that change.

Header changes

Now it's alright! :)
Updated IS with screenshots as well.