Problem/Motivation

If a developer is adding a CSS library, and forgets to nest the assets under one of the existing categories (component, theme, etc), the following error is thrown:

Error: Cannot use assign-op operators with overloaded objects nor string offsets in Drupal\Core\Asset\LibraryDiscoveryParser->buildByExtension() (line 138 of core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php).

this happens, for instance, with the following (incorrect) code in foo.libraries.yml:

bootstrap-cdn:
  remote: http://getbootstrap.com
  version: 3.3.6
  license:
    name: MIT
    url: https://github.com/twbs/bootstrap/blob/master/LICENSE
    gpl-compatible: false
  css:
    https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap.min.css: { type: external, minified: true }
  js:
    https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/js/bootstrap.min.js: { type: external, minified: true }
  dependencies:
    - core/jquery

Note the CSS needs to be nested like so:

...
  css:
    theme:
      https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap.min.css: { type: external, minified: true }

Proposed resolution

Add an assertion to make sure $options is an array.

Remaining tasks

User interface changes

API changes

Data model changes

Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

Status: Active » Needs review
FileSize
700 bytes

This adds an assertion which at least gives a hint as to what is happening. Another possibility would be to assert that the constant is defined.

dawehner’s picture

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?

jhedstrom’s picture

FileSize
1.27 KB
1.2 KB

This checks that the category constant is defined, and if not links to https://www.drupal.org/node/2274843.

Status: Needs review » Needs work

The last submitted patch, 4: 2705037-04.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
557 bytes
1.74 KB

This found an error in one of our test fixtures.

dawehner’s picture

Issue tags: +TX (Themer Experience)

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).

jhedstrom’s picture

This adds a test for the assertion. (I wasn't sure if there's a better way to check for failed assert() calls in PHPUnit or not.) The test-only file is the interdiff.

The last submitted patch, 8: 2705037-08-TEST-ONLY.patch, failed testing.

dawehner’s picture

+++ 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.

dawehner’s picture

Status: Needs review » Needs work
jhedstrom’s picture

Status: Needs work » Needs review
FileSize
537 bytes
2.24 KB
3.44 KB

re #11 I think the checks cannot both happen. If we check instead that $options is an array, that assertion will always fail before we then check valid category. On the other hand, if we check for valid category first, that will always fail before we check if $options is an array...

I have updated the test to fail more dramatically, as the IS shows.

The last submitted patch, 12: 2705037-12-TEST-ONLY.patch, failed testing.

dawehner’s picture

@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

dawehner’s picture

Status: Needs review » Needs work

I hope its okay to set it to needs work

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
4.07 KB
3.49 KB
4.75 KB

This doesn't quite check the max depth of the array (I wasn't sure how to readily implement that in this particular bit of code w/o adding a new method for max array depth). However, it does properly differentiate between bad categories and improper nesting.

jhedstrom’s picture

Hmm, this new re-work doesn't catch this failure:

bad_nesting:
  css:
    # No nesting here will break.
    css/styles.css: {  }

The last submitted patch, 16: 2705037-16-TEST-ONLY.patch, failed testing.

jhedstrom’s picture

What if we went back to a single assertion and just updated the message to something like:

Invalid CSS category (CATEGORY) or CSS may be improperly nested. See https://...?

I'm not seeing an elegant way to cover all the possible failures.

jhedstrom’s picture

FileSize
2.24 KB
5.74 KB

This adds a separate validation method as discussed with @dawehner at Drupalcon. This checks for improper option arrays, and then once those are verified, individual categories are asserted. Hopefully this results in clearer information for devs and frontenders.

dawehner’s picture

  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
    
jhedstrom’s picture

FileSize
5.09 KB
7.38 KB

I didn't know about setExpectedException. This seems much nicer. (Note, core is using a mix of both approaches at this time).

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.

jhedstrom’s picture

I wonder if there's a better way to move along these super-simple DX related fixes?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +DrupalWTF

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!

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
  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?

jhedstrom’s picture

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

It seems to me adding constants that are only used in an assert call would be unnecessary overhead?

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
5.21 KB
6.53 KB

I think this addresses the remaining feedback from #27. Good call on the data provider. Initial versions of the patch were using annotation for expected exceptions, for which a data provider wouldn't have worked.

I think originally example.js was just an oversight, so I've renamed that to example.css to avoid future confusion.

Wim Leers’s picture

Status: Needs review » Needs work

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!

Dinesh18’s picture

Status: Needs work » Needs review
FileSize
8.16 KB
1.4 KB

Here is an updated patch and interdiff which implemented #30

Wim Leers’s picture

Status: Needs review » Needs work

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 :)

Dinesh18’s picture

Status: Needs work » Needs review
FileSize
6.49 KB
2.57 KB

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Perfect! And no worries :) Thanks again!

larowlan’s picture

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

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed 04573c0 and pushed to 8.4.x.

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

  • larowlan committed 04573c0 on 8.4.x
    Issue #2705037 by jhedstrom, Dinesh18, dawehner, Wim Leers, larowlan:...
Wim Leers’s picture

Issue tags: +8.4.0 release notes

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