Diffs with different line endings leads to Invalid $mode 3 specified

Simple fix, correctly fixes the failed test and fixes our issues on a client project.

Left a question on the MR

Restoring status.

Thanks for working on this. It seems my initial implementation fell short on a number of things. Sorry for that.

The good thing is that we are adding test coverage for cases that were totally untested in the prior implementation - a more reliable pattern against possible future regressions.

In a couple of similar issues earlier, #3386482: DiffOpOutputBuilder does not correctly match the 10.0.x implementation and produces unexpected output and #3386707: DiffOpOutputBuilder does not correctly match the 10.0.x implementation and produces unexpected output (part 2), @alexpott also added the test case to the @legacy test for DiffEngine, so to show that the change is consistent with earlier behavior. I think it should be done here, too. NW for that.

OK, so now we have the regression fixed.

Since this is making a 'change' out of two exactly same lines that only differ by the EOL (being UNIX-like or WINDOWS-like), I was wondering: this is technically correct, but for a human it would make no difference. So I checked usage of Diff, and I find one only in core, where the strings are certainly stripped of the '\n' (but I am unsure about the '\r'), BEFORE they are sent for diffing. So this one might be an edge case. I think that would deserve some comments somewhere re. how the array of strings passed to Diff should be preprocessed for EOL markings.

From ConfigManager:

  /**
   * {@inheritdoc}
   */
  public function diff(StorageInterface $source_storage, StorageInterface $target_storage, $source_name, $target_name = NULL, $collection = StorageInterface::DEFAULT_COLLECTION) {
    if ($collection != StorageInterface::DEFAULT_COLLECTION) {
      $source_storage = $source_storage->createCollection($collection);
      $target_storage = $target_storage->createCollection($collection);
    }
    if (!isset($target_name)) {
      $target_name = $source_name;
    }
    // The output should show configuration object differences formatted as YAML.
    // But the configuration is not necessarily stored in files. Therefore, they
    // need to be read and parsed, and lastly, dumped into YAML strings.
    $source_data = explode("\n", Yaml::encode($source_storage->read($source_name)));
    $target_data = explode("\n", Yaml::encode($target_storage->read($target_name)));

    // Check for new or removed files.
    if ($source_data === ['false']) {
      // Added file.
      // Cast the result of t() to a string, as the diff engine doesn't know
      // about objects.
      $source_data = [(string) $this->t('File added')];
    }
    if ($target_data === ['false']) {
      // Deleted file.
      // Cast the result of t() to a string, as the diff engine doesn't know
      // about objects.
      $target_data = [(string) $this->t('File removed')];
    }

    return new Diff($source_data, $target_data);
  }

Also, I think additional test cases should be provided for DiffFormatterTest, that is currently only testing array of strings free from any EOL markings.

It seems like the additional tests are not coming quickly. Would maintainers consider fixing the bug without tests? Otherwise we are forcing everyone to live with a bug because we are worried that the bug might return one day.

For what it's worth, we've been running the patch from #3 above in production for nine months with no ill effects.

Rerolled for D11.3

The patch from MR4877 won't apply because /core/tests/Drupal/Tests/Component/Diff/Engine/DiffEngineTest.php doesn't exist in current Drupal 11.3.12. It also looks as if the patch for /core/tests/Drupal/Tests/Component/Diff/DiffOpOutputBuilderTest.php doesn't apply, although there are no errors (i.e. the code is unchanged after the patch with DiffEngineTest.php removed is applied).

This looks correct to me, the new tests look more than enough, thanks!

Committed and pushed cc28583dbe1 to main and e5dd13300b2 to 11.x and aa675a94345 to 11.4.x. Thanks!