LibraryDependencyResolver::getMinimalRepresentativeSubset() calculates dependencies incorrectly

I have created a test case which shows the failing issue and created a fix for the problem in the issue fork.

In general the problem is that before we did this:

foreach ($libraries as $library) {
  $with_deps = $this->getLibrariesWithDependencies([$library]);
  // We don't need library itself listed in the dependencies.
  $all_dependencies += array_diff($with_deps, [$library]);
}

The key problem is here:

$all_dependencies += array_diff($with_deps, [$library]);

I guess however wrote this assumes that this gives you the sum of the two arrays, but that is not the case, this only add the dependencies that exceeds the amount of dependencies we have of the left have side. The test case illustrates this.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

Created a MR from your branch so that the issue can be reviewed

Change I believe makes sense to avoid duplicates

Saving credits.

Finally got the test-only job to run and confirmed the results still match the previous test-only patch:

1) Drupal\Tests\Core\Asset\LibraryDependencyResolverTest::testGetMinimalRepresentativeSubset with data set #14 (array('test/deps_a', 'test/deps_d', 'test/no_deps_d'), array('test/deps_a', 'test/deps_d'))
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'test/deps_a'
     1 => 'test/deps_d'
+    2 => 'test/no_deps_d'
 )
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/project/drupal/core/tests/Drupal/Tests/Core/Asset/LibraryDependencyResolverTest.php:174
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/project/drupal/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/project/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/project/drupal/vendor/phpunit/phpunit/src/TextUI/Command.php:97
FAILURES!

Committed to 11.x, 10.2.x, and 10.1.x as a minimally disruptive data integrity bugfix. Thanks!

Wow! Looks like this was a regression introduced by #2407187: Optimize LibraryDependencyResolver::getMinimalRepresentativeSubset() and win >=4% → , since that changed the logic originally introduced by #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests → .

This likely reduced the ~4% performance improvement that #2407187 brought, but obviously correctness is more important.

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

So glad someone else was able to narrow down the problem here. I spent a long time on this in 🐛 Aggregated URL hashes for assets can mismatch due to different order of library assets Active but wasn't able to pin it down. Thank you!!