Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
We want Drupal 8 to be fast by default. One aspect of being fast by default is the front-end performance, and one aspect of that is the amount of data we send to the browser. JavaScript minification can be a huge help in that regard, but has long been impossible, for multiple reasons:
- The most fundamental blocker. Drupal <8 used to be developer-friendly by default rather than fast by default, which meant that if we'd ship with JS aggregation/minification enabled by default, we'd have to detect file system changes on every request, which would be detrimental for performance. #2226761: Change all default settings and config to fast/safe production values changes that: aggregation can be enabled by default, and when developing, it's easy to disable aggregation.
- The secondary blocker. The license problem: proper minification deletes all non-essential data, including license information. That's the part this issue addresses, in part. This issue is about adding the necessary metadata (because not every JS file contains license information, we cannot rely on that), which requires an API change.
- Exposing that license information: the closing stone for the license problem: #2258313: Add license information to aggregated assets.
- The last mile. We couldn't find consensus on which JS minification tool to use. This issue doesn't aim to solve that, it only aims to make it possible to add JS minification at a later point in time to Drupal 8, since adding that is not an API change, it's improving an existing feature. (Our current "minification" strategy is: "just use the entire file, and append a defensive semi-colon".) That could even happen after beta 1 (see #3).
Proposed resolution
Required reading:
- http://www.gnu.org/licenses/javascript-labels.html (referred to as "JSLWL" from now on)
- http://www.gnu.org/philosophy/javascript-trap.html#AppendixA
- Live examples of JavaScript License Web Labels: https://www.eff.org/librejs/jslicense, https://weblabels.fsf.org/www.fsf.org/CURRENT/
In essence, JSLWL requires us to list every JS file on the site, with its license and full source.
So, the proposed solution takes these steps:
- In Drupal 8, all assets must be declared through libraries. Libraries are declared in
<module name>.libraries.yml
files. We add a newlicense
key with a map containing the following key-value pairs:name
(license name),remote
(URL to the project's license) andgpl-compatible
(whether this license is GPL-compatible or not). e.g.:
backbone: remote: https://github.com/jashkenas/backbone version: 1.1.0 license: name: MIT remote: https://github.com/jashkenas/backbone/blob/1.1.0/LICENSE gpl-compatible: true js: assets/vendor/backbone/backbone.js: { weight: -19 } dependencies: - core/underscore
- Libraries without the
license
key are assumed to be "GPL2+", like Drupal, and are hence GPL-compatible. This means that none of the modules on drupal.org that include JavaScript code need to define licenses, unless they reference externally hosted JavaScript, which is a tiny minority. - This allows us to generate a "JavaScript License Information" page, as mandated by JSLWL. We will implement that in #2258313: Add license information to aggregated assets.
(Why not in this issue? Because it doesn't require API changes and contains some contentious things, whereas adding license metadata is not contentious.)
Remaining tasks
Review.
User interface changes
None.
API changes
- License information in
*.libraries.yml
files.
Comment | File | Size | Author |
---|---|---|---|
#17 | 2276219_17.patch | 2.7 KB | Mile23 |
#10 | asset_libraries_licenses-2276219-10.patch | 23.16 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
corbacho CreditAttribution: corbacho commentedIf test passes, this patch is ready IMHO. The license links were reviewed in the parent issue already:
https://drupal.org/node/2258313#comment-8749623
Comment #4
corbacho CreditAttribution: corbacho commentedPatch is missing the file
library_test_files/licenses.libraries.yml
?Comment #5
sunStill 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"?
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...
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.
Doesn't appear to belong to this patch?
Please remove the custom assertion messages from all of these.
Comment #6
Wim Leers#4: D'oh :( Fixed.
#5:
compatible
is so vague. Let's just rename it togplv2-compatible
? :)Actually, they are consistent. Whenever a specific tag is available, it points to that. Otherwise, it points to the best alternative.
That's not a new problem. People also tend to forget to update the
version
key.Comment #7
Wim LeersWhat else is left here? Let's get this in!
Comment #8
Wim LeersReroll; this was broken by #2223143: Consolidate library extension caches into a single cache entry.
Also verified that no 3rd party libraries have been added in the mean time; so this patch should once again be good to go.
Comment #9
dawehnerYeah to unit tests!
Doesn't all the code not run if buildByExtension throws an exception?
Comment #10
Wim Leers#9: good catch; you're absolutely right! I think that might be a remnant from an early iteration. Removed.
Comment #11
dawehnerThank you!
Comment #12
alexpottCommitted 256b197 and pushed to 8.x. Thanks!
Comment #14
nod_Doesn't this need a change record?
Comment #15
alexpottGood point - it does. I thought we might get away with updating https://www.drupal.org/node/2201089 but I don't think so
Comment #16
pbull CreditAttribution: pbull commentedPHPUnit 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()
?Comment #17
Mile23Here'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
Comment #18
dawehnerThese tests don't fail, unless you run them in static mode or code coverage, just to clarify.
Comment #19
alexpottOkay, committed 8d6f823 and pushed to 8.x. Thanks!
Can we get a change record now?
Comment #21
alexpottComment #22
alexpottComment #23
Wim Leers@Mile23: thanks for #17!
Change record published: https://www.drupal.org/node/2307387.
Comment #25
dawehnerAdd a related issue
Comment #26
YesCT CreditAttribution: YesCT commentedchanging 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.