Empty 'libraries:' in theme .info.yml file produces confusing PHP warning

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -313,6 +314,11 @@ public function rebuildThemeData() {
+      if (!is_array($theme->info['libraries'])) {
+        $name = $theme->info['name'];
+        throw new ConfigException(sprintf('The libraries configuration for theme %s is not an array.', $name));
+      }

So, maybe quietly make it an array if it isn't one then.

What exactly that test should do? If i'm going right then it should make sure that $info['libraries'] and themes->library are empty? or something else need to be done?

I working on it.

This patch #6 work for me.

#6 worked fine for me too.

Still needs tests.

Tests for #6.

+++ b/core/modules/system/tests/themes/test_theme_libraries_empty/test_theme_libraries_empty.info.yml
@@ -0,0 +1,12 @@
\ No newline at end of file

\No newline at end of file patch warning

+++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
@@ -138,6 +138,20 @@ public function testRebuildThemeData() {
+    catch (\Exception $e) {
+      $this->fail('Warning: Invalid argument supplied for foreach() thrown.');
+    }

Shouldn't we add a better description of the exception?

This patch try to solve #18

#19 looks good to me.

#19 works fine. It was applied successfuly.

@joyceg: Cool, but we need a little more in a review. Did you test with a bad theme file and observe the proper error? Did you execute the test on its own to prove it works? We don't know.

On a side note, please do not post images of console output. It is enough to say that the patch applies. We all know what that means.

The final release of 8.0.x is out so this is moved to 8.1.x.

Here it goes, patch attached addresses #24.

Fixes everything from #24, triggering testbot.

@Sagar Ramgade you forgot a period.

Reroll patch for testing

@Cottsert, interdiff attached which includes suggestion by @iStryker.

Back to NR. Fail was due to an interdiff posted as a patch.

The test method name does not reflect what it is testing.

The test asserts true is true, instead of using the pass() method.

The assertion messages in the test don't make sense: they call exceptions "warnings" and don't mention what this is about: the libraries key.

The actual change looks fine :)

Here's a patch that addresses @Wim Leers suggestions, and adds back the test .info.yml file that fell out of the patch a few rolls back.

The test asserts true is true, instead of using the pass() method.

ThemeHandler test cases are based on PHPUnit, which doesn't have a pass() method, so I've kept the assertTrue().

The assertion messages in the test don't make sense: they call exceptions "warnings"

"Warnings" is correct. The bug manifests as PHP warnings in the DB log.

There was a problem with adding the test .info.yml file. Trying again...

And again...

Hey! we are reviewing this issue at a code sprint from DrupalCamp Campinas 2016 Brazil.
http://campinas2016.drupalcamp.com.br

We reviewed the patch and worked. @sebas5384 @feraline @racarvalho1987

After run the coder we found a variable that was not being used and we removed it.

It's ready to be commited.

Thank you!

Reviewed patch at #45 and it works, marking as RTBC.

The proposed resolution in the IS says:

Either ignore that libraries is not an array at this point, or throw a meaningful exception. Either would improve the experience.

So this patch went with the second option.

Is that really desirable though? It means it's okay for a key without values to exist in a theme's *.info.yml file.

I think this change needs to be carefully considered. One could argue for either option. I think this compiles with https://en.wikipedia.org/wiki/Robustness_principle, so I'm personally fine with it (even though it makes me twitch as a developer who favors strictness).

Random fails in Drupal\field\Tests\Update\FieldUpdateTest. Re-testing and restoring previous status.

Same as #50

The same random failures are happening elsewhere too, so the errors seem to be unrelated to this patch specifically. See #2749955: Random fails in UpdatePathTestBase tests

#55: that is then fully consistent behavior. So, no more concerns then! :)

This is already RTBC, so maybe this is a late comment, but considering that this seems to share the same characteristics with #2730807: WSOD on admin/modules if description is set but is NULL in module.info.yml and #2558645: Malformed module.info.yml prevents install with a confusing error, we should look for a more robust and generic solution. Like using sane defaults for when any of the required entries are missing from a .info.yml file, or better yet, displaying a more helpful message for the admin to react to, rather than the WSOD.

So from a release management perspective, making this key interpret empty as empty, as it were, is mostly non-disruptive, and consistent with the behavior of some other keys. We also have @Cottser's signoff for the theme system.

However, in general, I too would prefer to fail meaningfully rather than silently. I also agree with @almaudoh's feedback above. The disadvantage of removing the exception here to silently accept an empty value is that we cannot then make it throw an explicit error again in a minor release. So, for example, if we went for a holistic solution for this and #2730807: WSOD on admin/modules if description is set but is NULL in module.info.yml and the like, we would just provide a BC fallback for the keys that do already allow empty values, but this issue would move us away from failing in a way that communicates something.

On the other hand, from a DX perspective, it is actually pretty convenient to just allow an empty template that you fill in, especially for such basic functionality as defining a new project and especially for the theme developer audience. From @Cottser:

This came up in real world usage commenting out a lone library in the .info.yml file for testing purposes.

To me, that is a compelling reason to allow empty non-required keys in .info.yml files in general. The only risk is that you don't realize you left it blank, but that seems a reasonable risk to me because if it's blank, and you go to the source and it seems blank, then that explains why it's blank. :)

That would also point to initializing the optional keys to array or emptystring or whatever at a higher level, rather than going through every place one of these keys is defined and adding a babysitting if (!empty())) check, but that is something that would work as a followup.

I've argued with myself back and forth a lot about whether to just commit this patch, which is probably a sign that we should indeed get that framework manager review. :) I do think we should at least move it to 8.2.x only. I'll reach out to the framework managers.

Thanks everyone!

(Saving credit.)

Adding @racarvalho1987 per #41.

I think this change is fine - the most important thing is that it behaves predictably - and I don't think this changes that. If you have an empty libraries key in your theme you don't expect it to break or load any libraries. If you have a broken library declared in the libraries key this doesn't affect that either so I'm +1 from a framework manager perspective.

Committed 2fb3a05 and pushed to 8.2.x. Thanks!

I think we should open a follow to address this in a little bit more of generic way... So both modules and themes have a default array to merge to the information in .info.yml - see _system_rebuild_module_data() and \Drupal\Core\Extension\ThemeHandler::rebuildThemeData(). I think it is worth considering that if the yaml value is NULL we should use the default. That would fix both #2730807: WSOD on admin/modules if description is set but is NULL in module.info.yml and #2594937: Empty 'libraries:' in theme .info.yml file produces confusing PHP warning