Asset library DX: Non-helpful fatal error if CSS isn't nested under an existing category

Oh yeah, that is super annoying if this happens. I wonder whether we could give a slightly better tip, aka. explaining them that a category/scope is needed with maybe a link to some documentation about it?

This is IMHO a quite good win for the theming experience!

I would be even better if we have a test for the assertion (just test for the thrown exception).

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -130,12 +130,14 @@ public function buildByExtension($extension) {
+            $category_weight = 'CSS_' . strtoupper($category);
+            assert('defined($category_weight)', 'Invalid CSS category: ' . $category . '. See https://www.drupal.org/node/2274843.');

IMHO it would be nice if we could distinct between the invalid category (but right level of nesting) and the case on which someone forgot about the nesting, given that this allow much better error messages.

@jhedstrom
Can't we just check for max depth of the array?

css:
  foo: {}

Treat this as 1

  css:
    foo:
      bar: 123

Treat this as 1

  css:
    theme:
      foo: {}

Treat this as 2

This should IMHO fix at least the common usecases

I hope its okay to set it to needs work

1. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -533,6 +533,48 @@ public function testLibraryWithLicenses() {
   +   *
   +   * @expectedException \AssertionError
   +   * @expectedExceptionMessage Invalid CSS category: bad_category. See https://www.drupal.org/node/2274843.
   ...
   +   *
   +   * @expectedException \AssertionError
   +   * @expectedExceptionMessage CSS must be nested under a category. See https://www.drupal.org/node/2274843.
   

   Note: https://thephp.cc/news/2016/02/questioning-phpunit-best-practices claims $this->setExpectedException should be used.

2. +++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/css_bad_category.libraries.yml
   --- /dev/null
   +++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/css_bad_nesting.libraries.yml
   
   +++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/css_bad_nesting.libraries.yml
   +++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/css_bad_nesting.libraries.yml
   @@ -0,0 +1,4 @@
   
   @@ -0,0 +1,4 @@
   +bad_nesting:
   +  css:
   +    # No nesting here will break.
   +    css/styles.css: { minified: true }
   diff --git a/core/tests/Drupal/Tests/Core/Asset/library_test_files/dependencies.libraries.yml b/core/tests/Drupal/Tests/Core/Asset/library_test_files/dependencies.libraries.yml
   

   I had an idea for another case which could happen easily and we maybe should test:

   css
     - css/styles.css
   

Thank you so much!

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -129,13 +129,17 @@ public function buildByExtension($extension) {
+          assert('\Drupal\Core\Asset\LibraryDiscoveryParser::validateCssLibrary($library[$type]) < 2', 'CSS files should be specified as key/value pairs, where the values are configuration options. See https://www.drupal.org/node/2274843.');
+          assert('\Drupal\Core\Asset\LibraryDiscoveryParser::validateCssLibrary($library[$type]) === 0', 'CSS must be nested under a category. See https://www.drupal.org/node/2274843.');
...
+            assert('defined($category_weight)', 'Invalid CSS category: ' . $category . '. See https://www.drupal.org/node/2274843.');

We need this so bad!

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -460,4 +464,34 @@ protected function resolveThemeAssetPath($theme_path, $overriding_asset) {
   +   *     - 0 if the library definition is valid
   +   *     - 1 if the library definition has improper nesting
   +   *     - 2 if the library definition specifies files as an array
   ...
   +        return 2;
   ...
   +          return 1;
   ...
   +    return 0;
   

   Is there any reason we can't use constants here instead of integers?

2. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -533,6 +533,63 @@ public function testLibraryWithLicenses() {
   +  public function testCssCategoryAssert() {
   ...
   +  public function testCssNestingAssert() {
   ...
   +  public function testCssNestingArrayAssert() {
   

   These three methods essentially duplicate the same logic - the only difference is the extension name and the exception message - Can we change to a single method with a @dataProvider?

3. +++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/css_bad_nesting_array.libraries.yml
   @@ -0,0 +1,4 @@
   +    # Specified as an array will break. ¶
   

   nit: whitespace at end of line here

4. +++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/dependencies.libraries.yml
   @@ -1,6 +1,7 @@
   +    theme:
   +      css/example.js: {}
   

   nice catch - question why this is a js file in a css blob though - perhaps it should be 'js' as key (no nesting required) instead of css?

Good call on the data provider indeed :)

Just one nit:

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
@@ -502,6 +502,52 @@ public function testLibraryWithLicenses() {
+      // Invalid category.
+      ['css_bad_category', 'Invalid CSS category: bad_category. See https://www.drupal.org/node/2274843.'] ,
+
+      // Improper CSS nesting.
+      ['css_bad_nesting', 'CSS must be nested under a category. See https://www.drupal.org/node/2274843.'],
+
+      // Improper nesting with bad key value pairs.
+      ['css_bad_nesting_array', 'CSS files should be specified as key/value pairs, where the values are configuration options. See https://www.drupal.org/node/2274843.'],

This can become

'Invalid category' => ['css_bad_category', …],
'Improper CSS nesting' =>
 […],
…

PHPUnit will then no longer say "Test case #1", but it will use that label!

Here is an updated patch and interdiff which implemented #30

Thanks! But then please also delete the now-pointless comments and whitespace.

1. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -502,6 +502,52 @@ public function testLibraryWithLicenses() {
   +      // Invalid category.
   ...
   +      // Improper CSS nesting.
   ...
   +      // Improper nesting with bad key value pairs.
   

   So remove these, and remove the newlines between them.

2. +++ b/core/tests/Drupal/Tests/Core/Asset/library_test_files/dependencies.libraries.yml
   --- /dev/null
   +++ b/interdiff.txt
   
   +++ b/interdiff.txt
   +++ b/interdiff.txt
   @@ -0,0 +1,21 @@
   
   @@ -0,0 +1,21 @@
   +diff --git a/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   +index 9df801eb3b..e6cc40b7f9 100644
   

   Also, we don't want the interdiff to be in the patch :)

Here is an updated patch and interdiff.txt
Sorry I haved added the interdiff in the patch.

Perfect! And no worries :) Thanks again!

Updating issue credits to include reviewers that shaped the final patch.

Committed 04573c0 and pushed to 8.4.x.

Thanks everyone for working on this, will make themer experience much nicer.

This will make such a huge DX difference. Tentatively tagging 8.4.0 release notes.