Problem/Motivation

The \Drupal\Component\Diff\DiffOpOutputBuilder implementation added in #3299678: Deprecate DiffEngine and replace with sebastian/diff doesn't match the ouput of 10.0.x diffing. This results in test failures for the update_helper module on 10.1.x.

Steps to reproduce

$a = <<<EOF
settings
settings::max_length : i:123;
status : b:0;
translatable : b:1;
EOF;

$b = <<<EOF
settings : a:0:{}
status : b:1;
translatable : b:1;
EOF;

$a = explode("\n", $a);
$b = explode("\n", $b);

$diff = new \Drupal\Component\Diff\Diff($a, $b);

dump($diff->getEdits());

On 10.0.x this produces:

^ array:2 [
  0 => Drupal\Component\Diff\Engine\DiffOpChange^ {#2855
    +type: "change"
    +orig: array:3 [
      0 => "settings"
      1 => "settings::max_length : i:123;"
      2 => "status : b:0;"
    ]
    +closing: array:2 [
      0 => "settings : a:0:{}"
      1 => "status : b:1;"
    ]
  }
  1 => Drupal\Component\Diff\Engine\DiffOpCopy^ {#2915
    +type: "copy"
    +orig: array:1 [
      0 => "translatable : b:1;"
    ]
    +closing: array:1 [
      0 => "translatable : b:1;"
    ]
  }
]

On 10.1.x this produces:

^ array:3 [
  0 => Drupal\Component\Diff\Engine\DiffOpChange^ {#1898
    +type: "change"
    +orig: array:2 [
      0 => "settings"
      1 => "settings::max_length : i:123;"
    ]
    +closing: array:2 [
      0 => "settings : a:0:{}"
      1 => "status : b:1;"
    ]
  }
  1 => Drupal\Component\Diff\Engine\DiffOpDelete^ {#1930
    +type: "delete"
    +orig: array:1 [
      0 => "status : b:0;"
    ]
    +closing: false
  }
  2 => Drupal\Component\Diff\Engine\DiffOpCopy^ {#1727
    +type: "copy"
    +orig: array:1 [
      0 => "translatable : b:1;"
    ]
    +closing: array:1 [
      0 => "translatable : b:1;"
    ]
  }
]

Proposed resolution

This is due to how DiffOpOutputBuilder changes a series of deletions and additions into a change.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3386482

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
StatusFileSize
new928 bytes
new676 bytes

There's a bug in \Drupal\Component\Diff\DiffOpOutputBuilder::toOpsArray when it determines what diff ops to add. Patch attached is just the minimum fix to show there is no test coverage. And patch added to show that the added test coverage works.

alexpott’s picture

alexpott’s picture

Priority: Normal » Major

Given this bug results in unexpected changes to diff produced by contrib modules I think that this is at least a major until proven otherwise.

Status: Needs review » Needs work

The last submitted patch, 3: 3386482-2.test-only.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review

The test only patch failed as expected and the other patch and MR passed as expected.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

RTBC the MR, the NW for test failure is due to failing test only patch.

A missing test case was added, and code changes do not impact the existing test cases. Therefore good to go for me.

mondrake’s picture

#8 xposted with #7

  • catch committed 51390a5e on 10.1.x
    Issue #3386482 by alexpott, mondrake: DiffOpOutputBuilder does not...

  • catch committed a5c3cdbd on 11.x
    Issue #3386482 by alexpott, mondrake: DiffOpOutputBuilder does not...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and cherry-picked to 10.1.x, thanks!

Status: Fixed » Closed (fixed)

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