Removed CSS files not removed from library definitions

CSS for files are called from classy as it was happening previously. I have not found any visual regression. Please check after screenshots.

After

Good stuff, thanks for the screenshots.

Had the same code in a couple other issues (#2586335: CSS file defined in file library but doesn't exist and #2586381: Incorrectly referenced editor.css in editor.libraries.yml) so I had already tested (almost) all this code. Works fine.

no apologies necessary, as long as it gets fixed I'm happy.

ah yes that would have been a good idea - I added the relationship the other direction which adds it to the child issues data but not as a post and hence is not any where near as noticeable (and might have meant you wouldn't have done extra work). thanks for the suggestion :)

Looks good to me

yeah that makes sense. Well hmm one question; Maybe it is normal but is there a reason to filter out contrib modules/themes? It seems to me it would be better to have the test run on them as well?

Makes sense to me and much less important than getting the actual problems fixed.
(and I feel like I might beshooting my mouth off and saying bigger things than I really should for my up Drupal hierarchy position... as in none - so feel free to ignore if I say anything out of turn)

1. +++ b/core/modules/system/src/Tests/Asset/LibraryCompletenessTest.php
   @@ -0,0 +1,193 @@
   +/**
   + * Tests that the files for all core libraries actually exist.
   + *
   + * @group Asset
   + */
   +class LibraryCompletenessTest extends KernelTestBase {
   

   <3

2. +++ b/core/modules/system/src/Tests/Asset/LibraryCompletenessTest.php
   @@ -0,0 +1,193 @@
   +      // Filter contrib, hidden, already enabled modules and modules in the
   +      // Testing package.
   +      if ($module->origin !== 'core' || !empty($module->info['hidden']) || $module->status == TRUE || $module->info['package'] == 'Testing') {
   

   <3

3. +++ b/core/modules/system/src/Tests/Asset/LibraryCompletenessTest.php
   @@ -0,0 +1,193 @@
   +    // overkill but themes can override and extend other extension's libraries
   

   Nit: s/extension's/extensions'/

4. +++ b/core/modules/system/src/Tests/Asset/LibraryCompletenessTest.php
   @@ -0,0 +1,193 @@
   +      $this->libraryDiscovery->clearCachedDefinitions();
   

   This should not be necessary. It's only necessary when modifying libraries, i.e. when changing some YML files.

This should not be necessary. It's only necessary when modifying libraries, i.e. when changing some YML files.

I said this because I interpreted this as "libraries-extend and libraries-override are cached once, and whenever we change the theme, we need to clear all caches", which would be wrong.

But my interpretation was wrong. What is actually happening is that LibraryDiscovery (library.discovery) is designed to be used within one request (service lifetime always matches request lifetime). Within a single request, you can't have multiple themes be active. So LibraryDiscovery caches thing statically (i.e. request lifetime). Which is correct. However, in this test, we basically simulate multiple requests. So we need to clear the static caches. Which is exactly what this does.

Sorry for the distraction!

That leaves only point 3 as a nitpick, which can be fixed on commit.

RTBC++

Discussed with @effulgentsia - we agreed this is an RC target cause D8 should not be produce 404s whilst loading library assets out of the box.

@Cottser nice looking test :)

1. +++ b/core/modules/system/src/Tests/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php
   @@ -0,0 +1,197 @@
   +        $libraries_to_skip = [
   +          // Locale has a "dummy" library that does not actually exist.
   +          'locale/translations',
   +        ];
   

   Does not need to be set in the for each - how about making it a class property.

2. +++ b/core/modules/system/src/Tests/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php
   @@ -0,0 +1,197 @@
   +    $modules = \Drupal::moduleHandler()->getModuleList();
   +    $extensions = $modules;
   +    $module_list = array_keys($modules);
   +    sort($module_list);
   +    $this->assertEqual($this->allModules, $module_list, 'All core modules are installed.');
   +
   +    $themes = $this->themeHandler->listInfo();
   +    $extensions += $themes;
   +    $theme_list = array_keys($themes);
   +    sort($theme_list);
   +    sort($this->allThemes);
   +    $this->assertEqual($this->allThemes, $theme_list, 'All core themes are installed.');
   

   These tests mean that this test will break if someone has a contrib module lying around. That doens't seem ideal. I think we should assert that the libraries array is not empty. And that should be okay.

3. +++ b/core/modules/system/src/Tests/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php
   @@ -0,0 +1,197 @@
   +    // By merging $modules and $themes into $extensions we're assuming that a
   +    // module and theme don't share names. If they did, their libraries would
   +    // conflict anyway.
   

   This has always been impossible.

Followup on my points
2. Yes but you are asserting against the output of $this->themeHandler->listInfo(); and \Drupal::moduleHandler()->getModuleList(); which can include more that core - no? In fact I'm not sure why test modules are not already breaking that assertion.
3. What I meant by impossible is that Drupal has never supported modules and themes having the same name. I don;t think that needs documenting here.

@Cottser - er point 2. yep you're right - carry on :)

The test is nice (maybe needs a followup to get a list of themes instead of hardcoding)

+++ b/core/modules/system/src/Tests/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php
@@ -0,0 +1,200 @@
+    sort($this->allModules);
...
+    sort($this->allThemes);

Nit pick: Might as well move the theme sort to the setup too.

Committed a1678a2 and pushed to 8.0.x. Thanks!

Yay, no more 404s!