As I discovered in comment #58 in #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works, drupal_get_css() will try to process an empty $css array when it could just return an empty string straight after the alter that allows modules to modify the array if it is still empty.

$css is generally only empty during ajax requests it seems.

Not returning early means it would be impossible for somebody to implement #theme 'styles' without causing exceptions on ajax requests, as the drupal_render() in drupal_get_css() would cause the theme system to initialise inappropriately.

Doing a quick empty() check would also be a slight performance boost for ajax pages.

This is probably minor in itself, but it's kind of blocking #2005970: In renderable arrays, #type should provide a #theme suggestion, similar to how drupal_prepare_form() works.

CommentFileSizeAuthor
#25 2032967.patch1.09 KBcatch
#24 2032967.patch689 bytescatch
#5 drupal_get_css_should-2032967-5.patch745 bytesjoelpittet
#3 drupal_get_css_should-2032967-3.patch541 bytesalansaviolobo
#1 2032967-1.patch509 bytesthedavidmeister

Comments

thedavidmeister’s picture

thedavidmeister commented
Status: Active » Needs review
StatusFileSize
new 2032967-1.patch509 bytes

patch.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Something has gone horribly wrong if drupal_add_css() is being called on AJAX requests. I wonder if this is actually masking a deeper bug.

alansaviolobo’s picture

alansaviolobo commented
Issue summary: View changes
StatusFileSize
new drupal_get_css_should-2032967-3.patch541 bytes

rerolling

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs review » Needs work

Something has gone horribly wrong if drupal_add_css() is being called on AJAX requests. I wonder if this is actually masking a deeper bug.

indeed. CNW.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new drupal_get_css_should-2032967-5.patch745 bytes

Bumping this to 8.1 shortly, It really needs to be sorted out why it's even getting that far with empty arrays.

Here's a hack to see if we can unearth some truths about this...

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Needs work

Let's see what happens but bumping to 8.1.x regardless

The last submitted patch, 5: drupal_get_css_should-2032967-5.patch, failed testing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Title: drupal_get_css() should return early if $css is empty. » AssetResolver::getCssAssets() should return early if $css is empty.
Issue tags: +Performance

Yup it's looks like all AJAX from first glance.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

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.

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.

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

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2032967.patch689 bytes

Redo of #5 to see if this still happens.

I wonder if this is purely happening when there's only a library or two to load, and the library only contains JavaScript - we don't know whether we need CSS or not until the library definitions have been parsed.

If that's the case, we can skip the sort and optimize, and cache the empty array, but it's absolutely fine and expected for that to happen on AJAX requests.

catch’s picture

catch
he/him
Primary language English
commented
Component: theme system » asset library system
Category: Bug report » Task
Issue tags: +Bug Smash Initiative
StatusFileSize
new 2032967.patch1.09 KB

And here's what the fix should look like. Not really a bug IMO but saves a bit of work.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Title: AssetResolver::getCssAssets() should return early if $css is empty. » AssetResolver::getCssAssets() should not try to sort and optimise if $css is empty.
Status: Needs review » Reviewed & tested by the community

#24 shows we have all sorts of test coverage hitting this, so no need to add more, change makes sense.

Updated the title to match the fix, we are not returning early because we want the cache to be set.

quietone’s picture

quietone commented

Checking RTBC issues. There are no unanswered questions here or other work to do. I did not review the change itself.

alexpott’s picture

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

Committed and pushed dc39dd56b6c to 11.x and e8961de11a5 to 10.1.x. Thanks!

  • alexpott committed dc39dd56 on 11.x 
    Issue #2032967 by catch, joelpittet, thedavidmeister, alansaviolobo:...

  • alexpott committed e8961de1 on 10.1.x 
    Issue #2032967 by catch, joelpittet, thedavidmeister, alansaviolobo:...
aaronbauman’s picture

aaronbauman
he/him
commented

For future googlers: the symptom that led me to this thread was "Exception: Invalid CSS asset type" error in dblogs.

Exception: Invalid CSS asset type. in Drupal\Core\Asset\CssCollectionRenderer->render() (line 82 of core/lib/Drupal/Core/Asset/CssCollectionRenderer.php).

This was is in the context of submitting a views exposed filter via ajax (including Better Exposed Filters, which I had originally suspected as the culprit).

patch in #25 fixed it.
I take it back. Not fixed. The AssetResolver still chokes on a null-set of CSS assets.

Anyone know offhand if there's another issue open for this already?

Turns out I had a bad hook_css_alter that was adding an invalid CSS asset, which led to the asset resolver choking. No patches necessary after all.

Status: Fixed » Closed (fixed)

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

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I'm quite sure we should have committed my exception from #5, lol

+    // This is likely an AJAX request, return early.
+    if (empty($css)) {
+      throw new \Exception('This should not ever happen, wonder if test produce this helvetica scenario.');
+    }
+

For the uninitiated https://www.youtube.com/watch?v=-Y-yKmzP-4U