Asset libraries should declare their license

If test passes, this patch is ready IMHO. The license links were reviewed in the parent issue already:
https://drupal.org/node/2258313#comment-8749623

Patch is missing the file library_test_files/licenses.libraries.yml ?

1. <code>
   +++ b/core/core.libraries.yml
   @@ -277,18 +293,30 @@ drupal.vertical-tabs:
   +    gpl-compatible: true
   

   Still wondering whether we can find a shorter name for this key...

   Since "gpl-compatible" is not fully accurate anyway (as it would have to be "gplv2-compatible"), perhaps we can shorten it to just "compatible"?

2. <code>
   +++ b/core/core.libraries.yml
   @@ -658,6 +743,10 @@ matchmedia:
   +    url: https://github.com/paulirish/matchMedia.js/blob/0.1.0/LICENSE.txt
   
   @@ -672,6 +765,10 @@ modernizr:
   +    url: https://github.com/necolas/normalize.css/blob/master/LICENSE.md
   

   The specified URLs are inconsistent — some are linking to the latest version in the mainline/master branch, some others are linking to the exact version of the included library.

   While it would probably be accurate to link to the version-specific license, I fear that people will forget to update this URL when updating a library...

3. <code>
   +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
   @@ -174,6 +174,15 @@ protected function buildLibrariesByExtension($extension) {
   +      // Assign Drupal's license to libraries that don't have license info.
   +      if (!isset($library['license'])) {
   

   Based on an earlier discussion in the previous issue, I think this logic should be hardened:

   1. If a library specifies a 'remote', then it MUST specify a 'license'. (should be enhanced to also require 'version' in a separate issue)

   2. If a library does not specify a 'remote' and 'license', then the default license is injected.

4. <code>
   +++ b/core/modules/system/system.routing.yml
   @@ -408,3 +408,11 @@ system.batch_page.json:
   +system.javascript_license_web_labels:
   

   Doesn't appear to belong to this patch?

5. <code>
   +++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryTest.php
   @@ -376,6 +376,88 @@ public function testLibraryWithDataTypes() {
   +    $this->assertEquals($library['license'], $expected_license, 'Expected license metadata found.');
   ...
   +    $this->assertEquals($library['license'], $expected_license, 'Expected license metadata found.');
   

   Please remove the custom assertion messages from all of these.

Yeah to unit tests!

+++ b/core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php
@@ -346,6 +346,123 @@ public function testLibraryWithDataTypes() {
+   * @expectedException \Drupal\Core\Asset\Exception\LibraryDefinitionMissingLicenseException
+   * @expectedExceptionMessage Missing license information in library definition for 'no-license-info-but-remote' in core/tests/Drupal/Tests/Core/Asset/library_test_files/licenses_missing_information.libraries.yml: it has a remote, but no license.
+   *
+   * @covers ::buildLibrariesByExtension()
+   */
+  public function testLibraryWithMissingLicense() {
...
+    $libraries = $this->libraryDiscoveryParser->buildByExtension('licenses_missing_information');
+    $library = $libraries['no-license-info-but-remote'];
+    $this->assertCount(1, $library['css']);
+    $this->assertCount(1, $library['js']);
+    $this->assertTrue(isset($library['license']));
+    $default_license = array(
+      'name' => 'GNU-GPL-2.0-or-later',
+      'url' => 'https://drupal.org/licensing/faq',
+      'gpl-compatible' => TRUE,
+    );
+    $this->assertEquals($library['license'], $default_license);

Doesn't all the code not run if buildByExtension throws an exception?

Thank you!

Committed 256b197 and pushed to 8.x. Thanks!

Doesn't this need a change record?

Good point - it does. I thought we might get away with updating https://www.drupal.org/node/2201089 but I don't think so

PHPUnit tests are failing with:

Trying to @cover or @use not existing method "\Drupal\Core\Asset\LibraryDiscoveryParser::buildLibrariesByExtension".

Should the annotations in core/tests/Drupal/Tests/Core/Asset/LibraryDiscoveryParserTest.php now be using @covers ::buildByExtension() instead of @covers ::buildLibrariesByExtension()?

Here's a patch to fix the errors @pbull mentions in #16.

Fixes some @covers annotation, and also test method names to reflect the method names being tested.

There really should be a flag in the phpunit.xml.dist file to check for bad coverage annotation within continuous integration. Discussion here: #2105583: Add some sane strictness to phpunit tests to catch risky tests

These tests don't fail, unless you run them in static mode or code coverage, just to clarify.

Okay, committed 8d6f823 and pushed to 8.x. Thanks!

Can we get a change record now?

Add a related issue

changing to use the more common tag, so the less common one can be deleted, so it does not show up in the auto complete and confuse people.