Validate the CSS categories in libraries.yml files.

I am not sure whether i am going in right direction so writting a pseudo before actually submitting a patch:
if ($type == 'css' && !empty($library[$type])) {
foreach ($library[$type] as $category => $files) {
+if(empty($file)){
+ throw exception 1 as above
+}
+if(!empty($category) && $category not in valid categories ) {
+ throw exception 2 as above
+}
foreach ($files as $source => $options) {
if (!isset($options['weight'])) {
$options['weight'] = 0;
}
Please tell the correct way.
Thanks

Yes, something like that is what we need. There's no need to feel afraid to post a patch, even if it's imperfect. We all post imperfect patches all the time — we all rely on the feedback from others will help get us there :)

Working on it

Thanks for working on this! Ping me on IRC if you have questions :)

I have made changes in LibraryDiscoveryparser.php and added the validations for invalid category.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -10,6 +10,8 @@
    use Drupal\Core\Asset\Exception\IncompleteLibraryDefinitionException;
    use Drupal\Core\Asset\Exception\InvalidLibraryFileException;
    use Drupal\Core\Asset\Exception\LibraryDefinitionMissingLicenseException;
   +use Drupal\Core\Asset\Exception\InvalidDefinationOfCssException;
   +use Drupal\Core\Asset\Exception\InvalidCssCategoryException;
   

   Nitpick: Please try to alphabetize the list.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -118,12 +120,21 @@ public function buildByExtension($extension) {
   +              throw new InvalidDefinationOfCssException(sprintf("You have to key the css files in the '%s'.library.yml file by category ('%s' seems to be a file name, allowed categories are Base, Layout, Component, State, Theme)", $extension, $category));
   

   Putting single quotes around %s in '%s'.library.yml might be confusing. On that note, I think the filename will be .libraries.yml.

3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -118,12 +120,21 @@ public function buildByExtension($extension) {
   +              //Check if the category is valid or not and
   +              // apply the corresponding weight defined by CSS_* constants.
   

   Nitpick: There should be a space after //.

4. +++ b/core/vendor/symfony/yaml/Symfony/Component/Yaml/Parser.php
   @@ -52,6 +52,7 @@ public function __construct($offset = 0)
   +        ¶
   

   Remove the extra whitespace.

Hi @hussaian,
Thanks for the feedback. I have corrected the issues mentioned above and resubmitting the patch.

Define the exceptions.

Fixed.

We discovered 2 tests (including one that dynamically adds a library) that doesn't provide a category.

It seems we were working on this at the same time. Anyway, I applied my remaining changes on top of your patch. Basically, I renamed the exception class to something slightly smaller and reformatted a block of code. Interdiff is attached.

Looks good to me. RTBC.

I wonder why the test has the extension .js

 example:
   css:
-    css/example.js: {}
+    theme:
+      css/example.js: {}

We could have kept it, since it shows that the file extension is not required to be the same as the category. But, I think it's just from a copy & paste.

It looks like the exception I renamed in #19 was not carried forward in #22. Any particular reason? There was no explanation in that comment.

I am not too comfortable with two exceptions either and think there might be a way to put it under an umbrella. They indicate two different things but they are very much related.

On first thought, I would think we could just use InvalidCssDefinitionException as that could indicate both a missing category and an invalid category. Thoughts?

For #24.

" Any particular reason?" My mistake.

That's okay. Did you have a thought on reducing it to one new exception as discussed in #23.1 and #24?

I think what you said in #24 is ok. The error message will still be different though, right? So, I think that works.

Made the changes as discussed. Both comments in #23 are addressed.

The new exception would need to be added to the throwing function's comment.

Would we actually want to catch this exception? It's too bad that the page will show a generic error if there is an exception.

Comment the throw.

Looking good, but we still want a unit test for this.

Specifically, we want a test for that in \Drupal\Tests\Core\Asset\LibraryDiscoveryParserTest. There are many examples to start from in that one already, so should be doable :)

Added Unit Tests.

Awesome!

Almost there :)

1. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -179,6 +179,48 @@ public function testBuildByExtensionWithMissingInformation() {
   +   * @expectedExceptionMessage You have to key the css files
   

   This message is pretty vague.

   Let's turn it into: "Every CSS asset must be nested under a SMACSS category, i.e. either theme,
   or base. See for details."

2. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -179,6 +179,48 @@ public function testBuildByExtensionWithMissingInformation() {
   +   * @expectedExceptionMessage You have used an invalid category
   

   Let's similarly expand this message.

We already say the categories in the exception. The @expectedExceptionMessage is only searching for a substring of the actual exception. I do like your wording better than saying "You have...". On the other hand, the way that the exception message currently reads, it tells you which file was wrong.

Also, do we want to catch this exception anywhere or catch it in a new issue?

Made advised edits comments as per #37

Fixing messages in #39 and added two more.

Novice task: review the patch in #41 as outlined in this contributor task: https://www.drupal.org/contributor-tasks/review

Adjusting positioning and content of beta review based on feedback by YesCT.

I applied the patch from #41 on a clean installation and I get:

Drupal\Core\Asset\Exception\InvalidCssDefinitionException: Every CSS asset must be nested under a SMACSS category in the core.library.yml file ('theme' seems to be a file name, allowed categories are Base, Layout, Component, State, Theme) in Drupal\Core\Asset\LibraryDiscoveryParser->buildByExtension() (line 136 of core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php).

Update: It also breaks using Simplytest.me

Just to make sure, when we say "validate the css categories", we only mean that people should group their css, right? We're not forcing them to use SMACSS naming, right? If company's have a different css architecture, they can still do:

css:
  kittens:
    kittens.css
  llamas:
    llames.css
  pandas:
    pandas.css

That's correct, right?

Let's say I'm using a third-party library (or a css framework) that I want to include. I'll always have to insert the css into a certain category … which might feel kinda strange. Let's say I'm using the Bootstrap library. If I were to use that framework, it contains css from all SMACSS categories. That means I'd do something like:

kittens.bootstrap:
  version: VERSION
  css:
    all: # 'Cause it's base, components, layout, …
      css/bootstrap.min.css: {}

I think this patch throws an exception if you don't use one of the SMACSS categories. We should make sure that's really what we want.

I think this patch throws an exception if you don't use one of the SMACSS categories. We should make sure that's really what we want.

Discussed this with Lewis Nyman yesterday and that is not what we want, especially since the official SMACSS terminology is modules while we ofter say components and theme while we often use skin, due to the fact that modules and themes have a specific meaning in Drupal.

People should also not be forced to use SMACSS categories. I think it started from Yahoo and it has proven to be very successful, but if people want to organise their css in a different way and/or with a different naming convention, they should be able to do so.

We're not forcing them to use SMACSS naming, right? If company's have a different css architecture, they can still do:

No, that's not correct, we are forcing SMACSS. Discussing that here is out of scope.

Regarding #45:

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -122,12 +123,21 @@ public function buildByExtension($extension) {
           foreach ($library[$type] as $category => $files) {
+            if (empty($files)) {
+              throw new InvalidCssDefinitionException(sprintf("Every CSS asset must be nested under a SMACSS category in the %s.library.yml file ('%s' seems to be a file name, allowed categories are Base, Layout, Component, State, Theme)", $extension, $category));
+            }

$library['css'] has a category that is empty that is causing the exception.

No, that's not correct, we are forcing SMACSS. Discussing that here is out of scope.

I don't understand, where are we defining the list of valid categories? Even if we are only implementing the enforcement here, this is a TX regression.

Allowing a category to be empty and adding a test.

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -132,6 +132,11 @@ public function buildByExtension($extension) {
+           if (defined('CSS_' . strtoupper($category)) && empty($files)) {

Reminder, fix the spacing on this line in next patch.

As Wim pointed out:

No, that's not correct, we are forcing SMACSS. Discussing that here is out of scope.

We currently agreed on forcing SMACSS, apparently. Can we open an issue to discuss this change (#52), before we start working on this? The SMACSS categories also define the order the css is loaded in. If you don't categorise the css, what will be the weight of the css? When will be loaded?

Can we keep this issue to forcing the SMACSS categories? If we want to move into another direction, make sure to open a discussion first - and than work on the patch. If I'm wrong, and we agreed somewhere to STOP using SMACSS, please refer to the issue where this has been agreed upon.

I don't understand, where are we defining the list of valid categories? Even if we are only implementing the enforcement here, this is a TX regression.

In common.inc:

/**
 * The default weight for CSS rules that style HTML elements ("base" styles).
 */
const CSS_BASE = -200;

/**
 * The default weight for CSS rules that layout a page.
 */
const CSS_LAYOUT = -100;

/**
 * The default weight for CSS rules that style design components (and their associated states and themes.)
 */
const CSS_COMPONENT = 0;

/**
 * The default weight for CSS rules that style states and are not included with components.
 */
const CSS_STATE = 100;

/**
 * The default weight for CSS rules that style themes and are not included with components.
 */
const CSS_THEME = 200;

+ in LibraryDiscoveryParser:

              // Apply the corresponding weight defined by CSS_* constants.
              $options['weight'] += constant('CSS_' . strtoupper($category));

If you don't categorise the css, what will be the weight of the css? When will be loaded?

Exactly.

1. +++ b/core/lib/Drupal/Core/Asset/Exception/InvalidCssDefinitionException.php
   @@ -0,0 +1,15 @@
   +class InvalidCssDefinitionException extends \RunTimeException {
   

   Let's rename this to InvalidCssCategoryException.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -131,12 +132,26 @@ public function buildByExtension($extension) {
   +            // Allow empty categories.
   +           if (defined('CSS_' . strtoupper($category)) && empty($files)) {
   +              unset($library[$type][$category]);
   +              continue;
   +            }
   +            if (empty($files)) {
   +              throw new InvalidCssDefinitionException(sprintf("Every CSS asset must be nested under a SMACSS category in the %s.library.yml file ('%s' seems to be a file name, allowed categories are Base, Layout, Component, State, Theme)", $extension, $category));
   +            }
   

   All of this can be replaced by:

   // Ignore categories that don't contain any files.
   if (empty($files)) {
     continue;
   }
   

All of this can be replaced by:

I'm not sure why it can be replaced.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -131,12 +132,26 @@ public function buildByExtension($extension) {
   +            // Allow empty categories.
   +           if (defined('CSS_' . strtoupper($category)) && empty($files)) {
   +              unset($library[$type][$category]);
   +              continue;
   +            }
   

   seven_library_info_alter() is unsetting the file name, but leaving the category. This part prevents an exception by skipping over the following code.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -131,12 +132,26 @@ public function buildByExtension($extension) {
   +            if (empty($files)) {
   +              throw new InvalidCssDefinitionException(sprintf("Every CSS asset must be nested under a SMACSS category in the %s.library.yml file ('%s' seems to be a file name, allowed categories are Base, Layout, Component, State, Theme)", $extension, $category));
   +            }
   

   This part throws a specific exception message, if a file is not under a category.

Ok, I figured we don't care about validating invalid categories if they don't contain anything. You want to still validate those. That's fine (better).

But then let's make the code easier to understand. Let's change what you currently have:

if (defined && empty) {
  …
}
if (empty) {
  …
}

to?

if (empty) {
  if (defined) {
    …
  }
  else {
    …
  }
}

Sure, and added a comment.

test it.

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -132,13 +132,16 @@ public function buildByExtension($extension) {
             if (empty($files)) {
...
+              else if (empty($files)) {

This nested if-test is always going to be true. No need to repeat it.

I processed the remark mentioned in #62.

Let's change this status to needs review

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
@@ -187,10 +187,72 @@ public function testBuildByExtensionWithMissingInformation() {
+   * @expectedExceptionMessage You must use a valid SMACSS category to nest a CSS asset
+   * @expectedExceptionMessage css_invalid_category

Nit: the first line wins, the second line can be removed. Can be done upon commit.

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
@@ -187,10 +187,72 @@ public function testBuildByExtensionWithMissingInformation() {
+     $path = __DIR__ . '/library_test_files';
+     $path = substr($path, strlen($this->root) + 1);
+     $this->libraryDiscoveryParser->setPaths('module', 'css_empty_category', $path);
+
+     $this->assertSame($this->libraryDiscoveryParser->buildByExtension('css_empty_category'), array());

Are we not missing a test file for this?

Removing sprint weekend tag!!
As suggested by @YesCT

Sorry, these issues were actually worked on during the 2015 Global Sprint
Weekend https://groups.drupal.org/node/447258

Rerolling the patch #63 and also address comment #65

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
@@ -213,7 +213,6 @@
    * @expectedException \Drupal\Core\Asset\Exception\InvalidCssDefinitionException
    * @expectedExceptionMessage You must use a valid SMACSS category to nest a CSS asset
-   * @expectedExceptionMessage css_invalid_category

This change looks good. The rest of the changes were maybe included by mistake.

I dont know why it's failing..
I am unable to debug!!
Can anyone help me with this?

those changes should not be part of the patch. Only the one change.

@lokapujya Ok thnx..
making that change only.

@lokapujya But these changes are in that patch.. :(
Still confused.

@deepakaryan1988 Noticed a syntax error.
$path = substr($path, strlen($this->root) 1);

Can you re-do the reroll of #63? The patch in #71 contains changes that should not be included. Only the change shown in #73 is correct.

#63 re-rolled with change mentioned in #73.

Needs a test file for css_empty_category similar to css_missing_category.libraries.yml.

Address the issue raised in #66

Including the interdiff so that we can review the changes.

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
@@ -187,10 +187,74 @@ public function testBuildByExtensionWithMissingInformation() {
+   * @expectedException \Drupal\Core\Asset\Exception\InvalidCssDefinitionException
+   * @expectedExceptionMessage Every CSS asset must be nested under a SMACSS category
+   * @expectedExceptionMessage css_missing_category
+   *

Looks like there is another test that has a duplicated @expectedExceptionMessage

Rerolling and also fixing issue in #87. Since it is just a small line change, it didn't make sense to separate patches and create an interdiff. I hope it's okay.

The whole purpose of the interdiff is so that we can see just the small change!

Looks good though.

Yay more tests! RTBC++

I have seen this error in other issues as well. I think it is to do with a bot or random. Retesting.

Back to RTBC then

Do we really want limit the categories to SMACCS categories? Couldn't we just define a weight for unknown categories and say they come after theme? i.e the lowest of the low.

Currently, SMACCS is already required. I think this issue is supposed to make it obvious that you did not specify a category.

One possible problem with this issue is that if the CSS is dynamically added without a category, then an exception can be thrown.

@alexpott I'd prefer if we could put the non SMACSS category in like the example, and people questioned this in the theme talk we did tonight in Vancouver user group. We can by defining aribrary CSS_ contstants but that is not cool but better than nothing...

But like @lokapujya mentioned in #99 it's already required.

Before anyone spends time on this actually want ot make sure it's still a valid task.

Since there hasn't been a follow up going to close out for now. If still a valid task please reopen.

Thanks all