LibraryDiscoveryParser::buildByExtension() doesn't validate that extensions exist

What would be the expected behaviour, maybe throw an exception? Is that a BC break? Is this a BC break we actually "care" about?

How about an assert instead of an exception?

that way there is no BC break for production sites

but users of dev sites get the appropriate early warning indication.

I would not consider this as BC break unless someone provides a good use case of using buildByExtension() against non existing extensions.

This solution looks good... a minor adjustment we need a @throws

I will fix the tests.

@martin107 sorry, I noticed your assignment too late. Feel free to review / fix this patch.

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -63,6 +74,8 @@ public function __construct($root, ModuleHandlerInterface $module_handler, Theme
+   * @throws \InvalidArgumentException
+   *   Thrown when the extension is not installed.

@@ -79,9 +92,12 @@ public function buildByExtension($extension) {
+        throw new \InvalidArgumentException("The following extension is not installed in your system: $extension.");

Two problems with this:

1. The message is too generic. Something like The library '%s' could not be found because its extension '%s' is not installed. would be better, because it'd be in line with the other exceptions.
2. This should not use InvalidArguemntException, but should add a new exception class: we already have 5 such specific exception classes, this should have one too. Perhaps a LibraryNotFoundException (this is very similar to a 404 page not found after all).

Other than that, this looks splendid! Once that's fixed, this is RTBC.

The library '%s' could not be found.

This method builds information for all libraries in an extension, not just for particular library.

LibraryNotFoundException

This is not quite accurate because we are not looking for a library but for extension. I propose something like this: ExtensionNotFoundException or ExtensionMissingException.

Sounds good!

@Chi regarding #9 -- Ah no problem - a happy accident. My solution was similar to yours - and it is always good to see another person's thinking.

Your solution to the AjaxTest showed me that my solution would not work!

Regarding LibraryDiscoveryParserTest::setup()

If I may criticise constructively setting a mock that always returns TRUE is a little too permissive.

My preference is to only return true when required - That has the benefit that when other test code accidentally get past the moduleHandler->moduleExists() method call then an extra error will be triggered.

I have also added LibraryDiscoveryParserTest::testUnistalledTheme() which covers our new exception explicitly.

Regarding the name of the exception

NotFound has different meaning in terms of http requests and "is the file found on disc".
Missing has similar issue for me.

So I look at the documentation surrounding themeExists and moduleExits and saw that exists is used as another word for installed.

PS
Also I am going to file a separate issue - the documented list of @throw statements is not upto date.

1. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -161,6 +175,21 @@ public function testInvalidLibrariesFile() {
   +   * @expectedException \InvalidArgumentException
   

   Shall we change this to ExtensionDoesNotExistException?

2. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -161,6 +175,21 @@ public function testInvalidLibrariesFile() {
   +  public function testUnistalledTheme() {
   

   The name can be more meaningful. We are not testing uninstalled theme and did not ever uninstall themes in this case. We just make sure that the exception is thrown if non existing extension was passed to buildByExtension() method. I propose testMissingExtension or test testNotExistingExtension and so on.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxTest.php
@@ -53,7 +53,7 @@ public function testDrupalSettingsCachingRegression() {
-    $fake_library = 'fakeLibrary/fakeLibrary';

So in case of Bartik it would read as follows:

Theme extension 'Bartik' could not be found because its extension 'Bartik' is not installed

Sounds a bit weird to me. Also note that this exception will be thrown for missing modules as well. Actually if the extension is missing we have no idea what type it is supposed to be. I suggest we return previous wording discussed above.

Thanks! Great teamwork here :)

1. +++ b/core/lib/Drupal/Core/Asset/Exception/ExtensionDoesNotExistEception.php
   @@ -0,0 +1,10 @@
   + * Defines a custom exception if a theme is not installed.
   

   s/a theme/an extension/

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -74,8 +75,8 @@ public function __construct($root, ModuleHandlerInterface $module_handler, Theme
   +   *   Thrown when the theme or module is not installed.
   

   s/theme or module/library's extension/

3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -96,7 +97,7 @@ public function buildByExtension($extension) {
   +        throw new ExtensionDoesNotExistException(sprintf("Theme extension '%s' could not be found because its extension '%s' is not installed",  $extension));
   

   You're passing only one argument: $extension, but there are two '%s' occurrences.

   Plus the message does not make a ton of sense. What about The extension '%s' is not installed.

4. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -172,6 +175,21 @@ public function testInvalidLibrariesFile() {
   +   * @expectedException \InvalidArgumentException
   

   This was not yet updated, and hence the test should fail.

5. +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
   @@ -172,6 +175,21 @@ public function testInvalidLibrariesFile() {
   +  public function testUnistalledTheme() {
   

   s/Uinstalled/Uninstalled/

PS
Also I am going to file a separate issue - the documented list of @throw statements is not upto date.

+1, thank you!

What about The extension '%s' is not installed.

What about is not installed OR is not available

Most of the things to fix are the results of myself being in a rush ... so it is down to me to fix.. I will sort this out tonight ( in about 4 hours )

#19

If I do for example drush en flag, without first downloading the contrib module. Then it is auto downloaded for me.
So when you say

What about is not installed OR is not available

"available" implies to me that it could be if some kind of helpful fallback mechanism were invoked. That is not what is suggested by the wording of the documentation above ThemeHandlerInterface::moduleExists() - or as far a I can see what happens in the underlying implementation. So for the moment I went with

The extension '%s' is not installed.

Thank you all for the corrections - I think I got to all of them

For our new tests I went with "testMissingExtension".

I just noticed a little flaw, which is out of scope for this issue.

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
@@ -161,6 +175,28 @@ public function testInvalidLibrariesFile() {
+   * @expectedException \Drupal\Core\Asset\Exception\ExtensionDoesNotExistException

Let's also assert the expected message.

Also, we don't use @expectedException anymore, PHPUnit is deprecating that.

Use setExpectedException() instead.

After that, this is RTBC IMHO.

If it is ok with you I would like to fix that up in a follow up

so that we can refactor all the other @expectedException()'s in the asset library system.

Well, you still need to add assert the message. NW for that.

I'm fine with doing the conversion in a follow-up, but I know that committers have demanded that we add no new code that uses the deprecated mechanism.

no new code that uses the deprecated mechanism

I am happy to be held to that line.... I will patch that here.

Perfect :)

@catch this sounds like a new Core development policy that needs to be discussed and adopted globally.
Stop creating new exceptions until Drupal 9 to keep BC.

For me this case is not BC breaking. Building library information for non-existing extension is certainly a bug. It's not like PHP notices which can just spam watchdog but still let site work.

1. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -79,9 +92,21 @@ public function buildByExtension($extension) {
   +        \Drupal\Component\Assertion\Handle::register();
   

   This line is not necessary.

2. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -79,9 +92,21 @@ public function buildByExtension($extension) {
   +        if(function_exists('drupal_set_message')) {
   +          drupal_set_message($message, 'error');
   +        }
   

   This must be removed.

3. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
   @@ -79,9 +92,21 @@ public function buildByExtension($extension) {
   +        return NULL;
   

   This can just be return;.

@catch: Sorry for not thinking of that!

#45: I went through the exact same train of thought when reviewing this.

Wouldn't an assert normally be: assert('$this->themeHandler->themeExists($extension)', $message);

But also I don't really get why we're doing both an assert and a trigger_error(). If we're going to trigger an error that should suffice, no?

I think this issue should be 'needs review' till the assert thing is decided on.

Well, a hard fail is what you want while developing. It's just that we can't have a hard fail for existing production sites that happen to run with assertions turned on. But that means we can't have this assert at all, period. Which means we must only have the trigger_error(…, E_USER_WARNING)?

do we support not having assertions on for development

I think we don't need to support that. It's already a best practice to use settings.local.php for development, and that enables assertions by default.

I'm not a fan of logging this as a warning, because that is far too easy to miss. I wouldn't think to go and check the error log after modifying my *.libraries.yml file…

I agree with @Wim Leers, let's go with just assertions. We should not treat libraries in some special way.

So we want #43, but with the trigger_error() removed.

Yes, sorry for all the confusion!

but zend.assertions = -1 by default. Maybe we need to add info to "Status report" with the recommendation to change php.ini settings, if using settings.local.php?

When using settings.local.php, assertions are enabled automatically through this:

assert_options(ASSERT_ACTIVE, TRUE);

The default value in php.ini is irrelevant.

So let's go with either:

  …
  $extension_type = 'theme'
}
else {
  assert(FALSE, sprintf('The extension "%s" is not available.', $extension));
}

or:

  …
  $extension_type = 'theme'
}
assert(isset($extension_type), sprintf('The extension "%s" is not available.', $extension));

#61: if that PHP7 behavior were true, then any Drupal 8 unit test involving assertions would fail. Since they don't, it must be working.

I like that @catch can now choose between either approach :)

Eh… why?

I'd bet this was a PHP 7 bug that has been fixed since. Retesting and re-RTBC'ing.

It's a shame we lost track of this for the past six months… :/

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -474,6 +474,7 @@ public function getThemeDirectories() {
   public function themeExists($theme) {
+    $this->rebuildThemeData();

It feels like rebuilding this information all the time is not the best approach. Is there no other way to fix it?

Found this issue trying to fix a test fail from #2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname(). I don't know if we still need this issue after #2347783 is done since ExtensionLists throw the UnknownExceptionExtension if they are called on a non-existent extension. Some parts of the patch here may still be useful.

Adding diff between 81 and 87 patches.

This patch no need reroll.

We should look closely at
#2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname()

Becuse it might be introducing more bugs about by not failing when you try to get the path but trying to invoke alter hooks on modules that are not enabled.

I will try to introduce a test in that other issue for this.

Probably the following issue should be marked as duplicate
#2279551: Library discovery Parser tries to guess extension type assuming 'theme' for non-existing modules

I can confirm that
#2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname()
Will solve this issue by throwing an Drupal/Core/Extension/Exception/UnknownExtensionException

What still bothers me is that I confirmed that
LibraryDiscoveryParser::buildByExtension()

will return information from themes that exist in the filesystem but are not active but for modules it will only return information for those that are enabled. When you query for a module that is in the filesystem but not enabled it will assume is a theme giving a confusing exception message like:

Drupal/Core/Extension/Exception/UnknownExtensionException with message 'The theme non_enabled_module does not exist.'

Should we repurpose this issue to fix that or open a new one?

I don't think this issue need to be postponed. Whichever of this and #2347783: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname() gets in first will cause a reroll of the other one, but the other issue doesn't fix this faulty logic:

      if ($this->moduleHandler->moduleExists($extension)) {
        $extension_type = 'module';
      }
// WRONG! Just because the module doesn't exist, doesn't mean it must be a theme!
      else {
        $extension_type = 'theme';
      }

+++ b/core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php
@@ -79,9 +92,10 @@ public function buildByExtension($extension) {
+      elseif ($this->themeHandler->themeExists($extension)) {
         $extension_type = 'theme';
       }
+      assert(isset($extension_type), sprintf('The extension "%s" is not available.', $extension));

we're also forgetting the profile extension type here

Closed #2279551: Library discovery Parser tries to guess extension type assuming 'theme' for non-existing modules as a duplicate, applying credit for