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.

Files: 
CommentFileSizeAuthor
#9 attach_only_libraries-2378095-9.patch27.9 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,891 pass(es). View

Comments

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Active » Needs review
FileSize
24.99 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
Wim Leers’s picture

Status: Needs review » Needs work

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

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
29.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,298 pass(es). View
5.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

FileSize
24.33 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,726 pass(es). View

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

#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

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

Status: Needs work » Needs review
FileSize
27.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,891 pass(es). View
4.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

nod_’s picture

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

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

Status: Fixed » Closed (fixed)

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