Problem/Motivation

This is a blocker for #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.

In the D8 cycle, we've worked hard to convert all asset handling into attached asset libraries. This is >97% complete. Getting to 100% would make core a consistent example, and would result in a single, clear DX, rather than a fragmented one.

Getting to 100% will also allow for #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 to happen.

Proposed resolution

Convert the remaining ones.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#9 interdiff.txt4.96 KBWim Leers
#9 attach_only_libraries-2378095-9.patch27.9 KBWim Leers
#6 attach_only_libraries-2378095-6.patch24.33 KBDevin Carlson
#5 interdiff.txt5.46 KBWim Leers
#5 attach_only_libraries-2378095-4.patch29.17 KBWim Leers
#2 attach_only_libraries-2378095-2.patch24.99 KBWim Leers
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue summary: View changes
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Assigned: Wim Leers » Unassigned
Status: Active » Needs review
FileSize
attach_only_libraries-2378095-2.patch24.99 KB
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Issue tags: +DX (Developer Experience)

Status: Needs review » Needs work

The last submitted patch, 2: attach_only_libraries-2378095-2.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Needs review
FileSize
attach_only_libraries-2378095-4.patch29.17 KB
interdiff.txt5.46 KB

Actually, there were only 6 failures. Testbot is currently failing due to #2376039: Undefined property ContainerAwareEventDispatcherTest::results in run-tests.sh.

Most failures due to two things:

  1. I forgot to include the new *.libraries.yml files in the previous patch. Fixed.
  2. Apparently Views' cache plugin did not yet support restoring attached libraries. Fixed that.
Devin Carlson’s picture

Devin Carlson CreditAttribution: Devin Carlson commented
FileSize
attach_only_libraries-2378095-6.patch24.33 KB

A reroll of #5 now that #2378789: Views output cache is broken has been committed which handled the conversion of views_test_data.libraries.yml.

The patch worked well in my testing; I verified that none of the assets were missing during installation, batch processing, on view pages, when modifying theme colour or creating/editing content. I also didn't find any attached CSS/JS that needed to be converted but was missed.

Wim Leers’s picture

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

#6 looks good to me, indeed only removes the now irrelevant hunks, everything else is unchanged.

Thank you very much for the manual testing, Devin!

If somebody could then still review the code, this can be RTBC :)

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Needs work

We can now remove color_css_alter.

The last patch mixes [] in places where array() is used. Can we have that consistent please?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs work » Needs review
FileSize
attach_only_libraries-2378095-9.patch27.9 KB
interdiff.txt4.96 KB

#8:

  1. We can now remove color_css_alter().

    Nice! :D

  2. I was trying to minimize changes, but sure :)
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Major » Critical

This is blocking #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 so bumping status.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Reviewed & tested by the community

Patch looks good. Tested views and all, still working.

I did find a views bug related to js events and lack of detach function but it's unrelated to this.

catch’s picture

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

Committed/pushed to 8.0.x, thanks!

  • catch committed 596978a on 8.0.x 
    Issue #2378095 by Wim Leers, Devin Carlson: Convert all remaining...
hass’s picture

hass CreditAttribution: hass commented

Is this issue the root cause of #2391025: Add support for inline JS/CSS with #attached?

Status: Fixed » Closed (fixed)

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