LibraryDependencyResolver::getMinimalRepresentativeSubset() accepts a set as the input but doesn't complain if it's not actually a set

Given the test fails in HEAD, I think it is a bug?

+++ b/core/lib/Drupal/Core/Asset/LibraryDependencyResolver.php
@@ -70,6 +70,8 @@ protected function doGetDependencies(array $libraries_with_unresolved_dependenci
+    assert('count($libraries) === count(array_unique($libraries))', '$libraries must be a set.');

I'm curious whether we could create a better asertion message. Many drupal developers are coming from backgrounds which might not be familiar with the property of sets you deal with here.

I tend to agree that set is pretty okay as error message, but better ideas welcome :).

Agreed with the other committers that this is an RC target, however:

1. Can we split adding the assert() to another issue, and leave this for the bug fix.

2. The test coverage looks incomplete?

If it goes into 8.3.x yeah.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDependencyResolver.php
   @@ -70,6 +70,8 @@ protected function doGetDependencies(array $libraries_with_unresolved_dependenci
   +    assert('count($libraries) === count(array_unique($libraries))', '$libraries must be a set.');
   

   The assertion message doesn't seem to reflect what is being checked.

2. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDependencyResolverTest.php
   @@ -174,4 +174,13 @@ public function testGetMinimalRepresentativeSubset(array $libraries, array $expe
   +   * @expectedException \AssertionError
   

   We use $this->setExpectedException() now..

The assertion message doesn't seem to reflect what is being checked.

Does it not? A set is a thing where each element is unique.

I think it's worth some redundancy in the assertion method. There are dozens of colloquial meanings of 'set' in English, including things like 'set of cutlery' or 'chess set' which explicitly have duplicate items. So I'd go for "$libraries must be a unique set" or something more explicit.

I think $libraries can't contain duplicate items. as an error message clearly states the intent without getting into semantics and helps developers understand what they need to do to resolve this.

I agree, this is a way nicer error message!

Committed 60c25e5 and pushed to 8.6.x. Thanks!

The assert is using the format deprecated in 7.2, so we need to update this one.

I think this satisfies #36.

Yep, back to RTBC

Committed b8c09be and pushed to 8.6.x. Thanks!

Only put in 8.6.x because we're just adding an assertion.