Problem/Motivation

Recently, several jQuery UI libraries were deprecated and had their assets refactored to make that possible: #3113400: Deprecate more jQuery UI library definitions.

That refactoring removed the jquery ui from dependency core/jquery.ui.dialog and called the assets directly instead. The refactoring didn't take into account the need to load core/jquery via the core/jquery.ui dependency. This was likely missed as the library isn't used by core anymore.

Steps to reproduce

Proposed resolution

Add test coverage that should have been there to catch something like this
Fix the issue by adding the necessary dependency to core/jquery.ui.dialog

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3191497-6.patch3.24 KBbnjmnm
#6 interdiff_2-6.txt3.33 KBbnjmnm
#6 3191497-6-TEST-ONLY.patch2.69 KBbnjmnm
#2 3191497.patch2.38 KBbnjmnm
#2 3191497-is-the-right-dependency-there-TEST-ONLY.patch1.37 KBbnjmnm
#2 3191497-are-dependencies-declared-at-all-TEST-ONLY.patch1.3 KBbnjmnm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bnjmnm created an issue. See original summary.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Priority: Major » Critical
Status: Active » Needs review
FileSize
3191497-are-dependencies-declared-at-all-TEST-ONLY.patch1.3 KB
3191497-is-the-right-dependency-there-TEST-ONLY.patch1.37 KB
3191497.patch2.38 KB

Bumping to critical since this could break direct uses of core/jquery.dialog (even if that would only be a problem in instances where no other jquery ui libraries were used)

The last submitted patch, 2: 3191497-are-dependencies-declared-at-all-TEST-ONLY.patch, failed testing. View results

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Related issues: +#3191527: Dialog width and positioning issues in Claro
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Title: core/jquery.ui.dialog missing a jquery ui dependency » core/jquery.ui.dialog is missing core/jquery assets
Issue summary: View changes
Status: Needs review » Needs work

The problem more accurately due to the missing core/jquery library, changing the IS to reflect that and will provide a new patch that adds jQuery instead of jQuery UI.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
Related issues: -#3191527: Dialog width and positioning issues in Claro +#3192804: Possibly undoing most of jquery.ui.dialog's dependency-detachment
FileSize
3191497-6-TEST-ONLY.patch2.69 KB
interdiff_2-6.txt3.33 KB
3191497-6.patch3.24 KB

Changed the jquery.ui.dialog dependency to core/jquery as that is actually what is needed and opened a followup to consider un-detaching jquery.ui.dialog, provided there are not compatibility concerns: #3192804: Possibly undoing most of jquery.ui.dialog's dependency-detachment

The last submitted patch, 6: 3191497-6-TEST-ONLY.patch, failed testing. View results

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs review » Reviewed & tested by the community

Thank you for opening the follow-up and adding the additional test coverage! Adding dependency on jQuery rather than jQuery UI seems less disruptive since there's some duplication across the assets between jquery.ui and jquery.ui.dialog.

  • catch committed 1e1d9a9 on 9.2.x 
    Issue #3191497 by bnjmnm, lauriii: core/jquery.ui.dialog is missing core...
catch’s picture

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

Committed 1e1d9a9 and pushed to 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

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