Problem/Motivation

#2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts that LibraryDependencyResolver::getMinimalRepresentativeSubset() may return a list rather than a set of libraries: i.e. it may return duplicates.

But this only happens if you pass in invalid data in the first place: if the input you pass in is already not a set.

Proposed resolution

Add an assertion that prevents that from happening, which then warns the developer the input is invalid.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#37 2594669-37.patch2.42 KBborisson_
#37 interdiff-2594669-30-37.txt741 bytesborisson_
#30 2594669-29.patch2.42 KBborisson_
#30 interdiff-2594669.txt1.56 KBborisson_
#21 interdiff.txt945 bytesWim Leers
#21 2594669-21.patch2.65 KBWim Leers
#8 interdiff.txt975 bytesWim Leers
#8 2594669-8.patch2.6 KBWim Leers
#2 2594669-2.patch1.65 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue summary: View changes
Status: Active » Needs review
FileSize
2594669-2.patch1.65 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: LibraryDependencyResolver::getMinimalRepresentativeSubset() may return the same library multiple times » LibraryDependencyResolver::getMinimalRepresentativeSubset() accepts a set as the input but doesn't complain if it's not actually a set

Better title.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +DX (Developer Experience)
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Category: Bug report » Task

The fact that this is merely a developer's mistake means this is not actually a bug, but a task.

Status: Needs review » Needs work

The last submitted patch, 2: 2594669-2.patch, failed testing.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Category: Task » Bug report
Priority: Minor » Normal

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Priority: Normal » Minor
Status: Needs work » Needs review
FileSize
2594669-8.patch2.6 KB
interdiff.txt975 bytes

I think all those failures are due to one and the same bug.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and for Drupal Association commented 
+++ 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.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I think we can assume developers know one of the most basic data structures, that are even part of primary school math. I might be mistaken though.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +rc target triage

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

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -rc target triage +rc target

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?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -rc target

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

Since this is no longer an RC target, I think it no longer makes sense to split this up.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.2.x-dev » 8.3.x-dev

If it goes into 8.3.x yeah.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Of course.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
  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..

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and for Drupal Association commented

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.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community
Related issues: +#2647916: Views ajax modals stop working in certain scenarios: due to JS settings caching, AJAX responses may set wrong ajaxPageState.libraries, causing problems for subsequent AJAX requests/responses
FileSize
2594669-21.patch2.65 KB
interdiff.txt945 bytes
  1. #19.1: see #20.
  2. #19.2: fixed.
  3. Also rebased, because conflicted with #2647916: Views ajax modals stop working in certain scenarios: due to JS settings caching, AJAX responses may set wrong ajaxPageState.libraries, causing problems for subsequent AJAX requests/responses.
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

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.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Needs review

"unique set" makes no sense at all.

What about: $libraries must be a set data structure?

So, this: https://en.wikipedia.org/wiki/Set_(abstract_data_type). Funny enough, no set in SPL: http://php.net/manual/en/spl.datastructures.php.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

@catch? :)

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented

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.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
FileSize
interdiff-2594669.txt1.56 KB
2594669-29.patch2.42 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

I agree, this is a way nicer error message!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

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

  • catch committed 60c25e5 on 8.6.x 
    Issue #2594669 by Wim Leers, borisson_, dawehner, catch:...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I thought this went in a long time ago! 😅 Thanks @borisson_ for pushing it to completion!

  • catch committed 3134f5c on 8.6.x 
    Revert "Issue #2594669 by Wim Leers, borisson_, dawehner, catch:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Fixed » Needs work

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

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff-2594669-30-37.txt741 bytes
2594669-37.patch2.42 KB

I think this satisfies #36.

Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Yep, back to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed b8c09be and pushed to 8.6.x. Thanks!

  • alexpott committed b8c09be on 8.6.x 
    Issue #2594669 by Wim Leers, borisson_, catch, dawehner:...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

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

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

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

WFM 👍

Status: Fixed » Closed (fixed)

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