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:

  1. 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.
  2. The secondary blocker. Far too often, we don't know and cannot determine the license of an asset! #2276219: Asset libraries should declare their license addresses that.
  3. The tertiary blocker. The license problem: proper minification deletes all non-essential data, including license information. That's the part this issue addresses.
  4. 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:

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:

  1. #2276219: Asset libraries should declare their license associates the license information with each asset library.
  2. This allows us to generate a "JavaScript License Information" page, as mandated by JSLWL. We link to that page on every Drupal page that contains any JS. And on that page, we can list every JS asset along with its license, thanks to 1.
  3. However, we must also list the license and source for aggregated JavaScript assets (which we want to be minified in the future). JSLWL allows us roughly 2 ways of doing that:
    The source code file can be a single, unminified JavaScript file, a .tar.gz archive, or a .zip archive. If a source archive includes multiple JavaScript files, the archive must include a file named 00-INDEX that lists the order in which individiual source files should be concatenated to produce a single file that's equivalent to what's hosted on the site.

    So either: an unminified aggregate, or an archive containing all aggregated JS files and a describing file. The latter is much more painful to do, so we choose the first, for which we already have the necessary infrastructure. The JS asset collection optimizer (asset.js.collection_optimizer) tracks its aggregated (and minified) JS files (we can get them via AssetCollectionOptimizerInterface::getAll()); by adding a secondary JS asset collection optimizer asset.js.collection_optimizer_license_web_labels_annotator which annotates each contained asset (with a link to the license information) without minifying it, we effectively get two key-value maps with the same keys (because the same collections of JS assets are passed to them and they use the same JS collection grouper) but with different files (one minified, another annotated and unminified). On the JSLWL "JavaScript License Information" page, we can now list the unminified annotated aggregate as the source for the minified aggregate.

Remaining tasks

Review.

User interface changes

None that are visible, but a new "JavaScript License Information" page at /system/jslicense.

API changes

  1. (Not really an API change, but an internal behavior change.) An unminified aggregate is generated for every minified aggregate.
  2. Route/response addition: /system/jslicense.
  3. (Not really an API change, but a markup addition.) Each page has a <link rel="jslicense" href="/system/jslicense" /> tag in the HTML head.

Comments

Wim Leers’s picture

Status: Active » Needs review
FileSize
36.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,350 pass(es). View
Wim Leers’s picture

Issue summary: View changes

To clarify: the goal is for this issue to allow us to add JS minification at a later point in the Drupal 8 cycle. That could be after beta 1 (but before RC) or even Drupal 8.1.0 or 8.2.0.

Wim Leers’s picture

#2226761: Change all default settings and config to fast/safe production values landed! If we can get this in also, we can add JS minification to D8 at a later time.

corbacho’s picture

Status: Needs review » Needs work

I checked every License link and they are valid working links with license info.

  1. +++ b/core/core.libraries.yml
    @@ -279,18 +295,30 @@ drupal.vertical-tabs:
    +    name: GPL2
    

    GPL2 should be "GNU-GPL-2.0-or-later"

  2. +++ b/core/core.libraries.yml
    @@ -342,7 +386,11 @@ jquery.once:
    -  version: &jquery_ui 1.10.2
    

    At least to me looks more clear with "jquery_ui_version" than "jquery_ui"

  3. +++ b/core/core.libraries.yml
    @@ -667,6 +752,10 @@ matchmedia:
    +    remote: https://github.com/Modernizr/Modernizr/blob/v2.6.2/readme.md
    

    Maybe direct link? http://www.modernizr.com/license/

  4. +++ b/core/core.libraries.yml
    @@ -674,6 +763,10 @@ modernizr:
    @@ -691,6 +784,9 @@ picturefill:
    

    Missing license info for picturefill and matchmedia ?

  5. +++ b/core/includes/theme.inc
    @@ -2082,6 +2082,14 @@ function template_preprocess_html(&$variables) {
    +      '#markup' => l(t('JavaScript license information'), 'system/jslicense', array('attributes' => array('rel' => 'jslicense', 'class' => 'hidden'))),
    

    A link in every page? This is excessive IMHO, but at least is hidden (although the rules are: (copy/paste) "This link can be small, but it should be clearly visible to people who visit your site.".

  6. +++ b/core/lib/Drupal/Core/Asset/LibraryDiscovery.php
    @@ -249,6 +258,13 @@ protected function buildLibrariesByExtension($extension) {
    +          if ($type === 'js' && !$library['license']['gpl-compatible']) {
    

    This is excessive IMHO. Does this means that every js library that does not have the 'gpl-compatible' tag will affect negatively the performance of the site? What about non-external libraries, like drupal-ajax ? Are they bundled together?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
37.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,393 pass(es). View
1.46 KB

Thanks for the review!

  1. Changed.
  2. Agreed, that's why I made the change. You're just +1ing this renamed YAML alias, right?
  3. Thanks, changed.
  4. I figured that made most sense because we currently have modified versions of those two libraries. But yes, let's add the license information now. Fixed.
  5. This is what the FSF, EFF etc. sites do, and what the standard prescribes. There was talk of using a <link> element instead. Though that's not yet in the standard, IMHO we could do that too, because it enables the same thing: automated discovery of JS asset licenses.
  6. drupal.ajax is GPL-compatible, so of course it's aggregated. Everyt JS asset used in Drupal core is GPL-compatible, and almost every JS asset in modules or themes on drupal.org is. This is necessary to be able to describe the license of aggregated JS assets.
    I could go one step further, and allow non-GPL-compatible assets also to be aggregated together, but in a separate group, so that we'd still be able to determine the license of JS aggregates.
corbacho’s picture

2. Indeed +1 the renamed the YAML alias to "jquery_ui_version". (I understood the opposite by mistake)
And, yes, I really like this patch and the motivation behind, in the meanwhile doesn't affect negatively the performance. Thank you for working on this.

moshe weitzman’s picture

Looks good. Two minor questions ...

  1. +++ b/core/includes/theme.inc
    @@ -2059,6 +2059,14 @@ function template_preprocess_html(&$variables) {
    +
    +  // On each page that uses JavaScript, add the mandatory JavaScript Web License
    +  // Labels link.
    +  if (count(drupal_get_js('header')) || count(drupal_get_js('footer'))) {
    +    $variables['page_bottom'][] = array(
    +      '#markup' => l(t('JavaScript license information'), 'system/jslicense', array('attributes' => array('rel' => 'jslicense', 'class' => 'hidden'))),
    +    );
    +  }
    

    A render array is better added in hook_page_build() IMO but it is a minor distinction.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.php
    @@ -38,6 +38,11 @@ public static function getInfo() {
    +
    +    // Ensure the system.javascript_license_web_labels route is available.
    +    $this->installSchema('system', array('router'));
    +    \Drupal::service('router.builder')->rebuild();
    +
    

    Why is this needed? Do we really start tests without a router table?

sun’s picture

  1. +    name: MIT
    +    name: Public Domain
    +    name: GNU-GPL-2.0-or-later
    

    Hm. This adds many custom license names, for which no schema/definition/documentation seems to exist.

    For example, you've added "Public domain", but that doesn't exist on http://www.gnu.org/licenses/javascript-labels.html

    At minimum, I think we need to add a license name validation to the processing code (throwing an exception when encountering an unknown license name).

  2. +    remote: https://github.com/jashkenas/backbone/blob/1.1.0/LICENSE
    

    The existing 'remote' key name refers to a "git remote repository".

    This isn't a remote, it's just a 'url'.

    I also wondered whether it shouldn't be 'source', which would be in line with http://www.gnu.org/philosophy/javascript-trap.html#AppendixA, but I guess 'source' might be confusing...

  3. +    gpl-compatible: true
    

    I wonder whether this key is really necessary?

    Why can't we derive that flag automatically from the license name?

  4. +    name: GNU-GPL-2.0-or-later
    +    remote: http://malsup.github.com/gpl-license-v2.txt
    +    gpl-compatible: true
    

    The issue summary states that GPLv2+ is the default, because it's Drupal's overall license, so why are we specifying it?

  5. +    name: GNU-GPL-2.0-or-later
    

    All of composer.json, package.json, and bower.json have a license key in their schema.

    I wonder whether we shouldn't use the license names of those much more common schemata instead and translate them into JSLWL identifiers?

Wim Leers’s picture

FileSize
37.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,480 pass(es). View
8.61 KB

Thanks for the reviews!

#8:

  1. Agreed, and that's what I did first. But that causes slightly different markup to be output: it causes a "region" wrapper <div> to be created, which is not the case with this approach. Since it's a hidden link anyway, I think it's better to not create the "region" <div>.
  2. Because it's a DUTB test.

#9:

  1. The license names listed at http://www.gnu.org/licenses/javascript-labels.html are not mandatory, they're considered "good" ( Good license identifiers and their associated links are: […]). Obviously, the list they have there is not even close to comprehensive.
  2. Aha! Yeah, I wanted to keep consistency, I never realized "remote" stood for "remote git repo". Thanks for that, I agree that "url" is much better :) I like your thinking regarding "source", but I agree it'd be confusing. So I went with "url".
  3. Because there are many GPL-compatible licenses, and aside from that, there are numerous licenses. We can't possibly include a comprehensive list to do this automatically.
  4. Good point. I figured we wanted to explicitly define the licenses of vendor libraries, because we (Drupal) don't own them. That removes any ambiguity. I don't have a strong opinion though, so if you want me to change that, I'll do it.
  5. Again, JSLWL doesn't have enforced "license identifiers"; it's free-form. Easily demonstrated by looking at https://weblabels.fsf.org/www.fsf.org/CURRENT/, where they don't use "GNU-GPL-2.0-or-later", but "GNU General Public License version 2 or later".

P.S.: wow, I didn't realize "schemata" was an alternative plural form of "schema"! :D

corbacho’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php
    @@ -81,11 +91,13 @@ public function optimize(array $js_assets) {
         // Now optimize (concatenate, not minify) and dump each asset group, unless
         // that was already done, in which case it should appear in
    -    // system.js_cache_files.
    +    // system.js_optimized_aggregate_files (but also the unoptimized equivalent
    +    // in system.js_unoptimized_aggregate_files, for JavaScript Web License
    +    // Labels compliance).
    

    The added comment doesn't read very easily, or is it me?. There is no verb. Changing "but also" to "and also" sounds better?

  2. +++ b/core/modules/system/lib/Drupal/system/Controller/AssetLicenseInfoController.php
    @@ -0,0 +1,158 @@
    +        $license_column = l($library['license']['name'], $library['license']['remote']);
    

    still 1 more "remote" left

About sun's 4. By using license 'name' in core, we give example to contrib. Also, what if Drupal changes to GPL3 at some point? But no strong opinion here.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
37.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,662 pass(es). View
2.29 KB
  1. Agreed; rewritten that comment. Should be much clearer now.
  2. Well-spotted, thanks!

RE: GPL3: that won't be a problem, since Drupal is already marked as "GPL2+". The only difference it will make here is that our aggregates would be labeled GPL3, and we could also include Apache-licensed JS assets in our JS aggregates. (The biggest difference license-compatibility wise is the Apache license compatibility.)

catch’s picture

I think we should explicitly add licenses for external libraries even if it's GPL2, otherwise you're left wondering.

Wim Leers’s picture

#13: indeed.

So, any other concerns? (I think I addressed all feedback so far?)

Bojhan’s picture

Its unclear to me if there are UI changes. None are visible, but we do add a page?

Wim Leers’s picture

#15: Correct. At /system/jslicense.

catch’s picture

The state entry can't necessarily be relied upon - at least the proposal to aggregate based on libraries in #1014086: Race conditions, stampedes and cold cache performance issues with css/js aggregation would remove the need to write aggregates to state since we could determine the aggregate to serve based entirely on URL. Even if we don't fix that in core before 8.0.0, the system is pluggable (i.e. agrcache and advagg both swap it out for 7.x already and neither use the core aggregate variable).

@Bojhan yes this will add a page, and possibly a visible link to that page on every other.

Wim Leers’s picture

#17:

If #1014086: Race conditions, stampedes and cold cache performance issues with css/js aggregation lands, we could just list all JS assets in the files/js directory; they'd all have the GPL2+ license anyway. We could then render cache for, say, 5 minutes (or more), to prevent FS hits every time /system/jslicense is loaded.

However, you're absolutely right that asset aggregation plugin overrides would break this. I don't see any other way of fixing that other than adding a new API to list all known aggregates, unfortunately.
Which makes me think we should we descope this issue a little bit: introduce license metadata for asset libraries here, expose /system/jslicense for each JS asset, but don't provide license information for aggregates just yet, because that requires a new API.

Thoughts?

Wim Leers’s picture

Marked #156124: JS and CSS aggregation deletes license information as a duplicate.

Can I has reviews? :)

hass’s picture

It sounds really bad to me that incompatible licenses should not be aggregated together in one file and causing several http requests in a browser. That may makes me setting a non-gpl libs as true just to make it compressed in one file. The intention of #156124: JS and CSS aggregation deletes license information was to keep the license above the code (remote value). Performance is more important.

Wim Leers’s picture

Title: Implement JS Web License Labels to allow for JS minification; each asset library should declare its license » Implement JS Web License Labels to allow for JS minification
Issue summary: View changes
Status: Needs review » Postponed
Issue tags: -beta target, -sprint
FileSize
37 KB

#20: You're right.

So, this patch essentially still has two problems:

  1. reliance on the system.js_optimized_aggregate_files entry in State, which shouldn't be relied upon because it's an overridable implementation detail (#17)
  2. negative performance impact when a site uses non-GPL-compatible JS (#20)

I propose we split this issue up in at least two parts:

  1. Introduce the license metadata for *.libraries.yml files — i.e.: the API change.
  2. Implement JS Web License labels, for which we will still need to solve problems 1 and 2 above — this will not change APIs.

That way, we increase our chances of success. I think all of us want to see Drupal 8 ship with JS minification. So let's get the first point done.


I've opened #2276219: Asset libraries should declare their license and posted a patch. Please review that!

This issue is now blocked on #2276219. Attached is a straight reroll of #12, which should not be reviewed — I'm only posting to make it easier to continue working on this once #2276219 lands.

hass’s picture

We also do not care about CSS framework licenses for many years and just remove all comments (including the license comments). This cannot be a blocker for JS minification. Otherwise we need to remove CSS aggregation also as long as this feature is in. The ONLY reason why we have removed JS minification from core in past was that *valid* JS code has been destroyed by the minification.

I can search for the case ID if this is of interest.

Wim Leers’s picture

Title: Implement JS Web License Labels to allow for JS minification » [PP-1] Implement JS Web License Labels to allow for JS minification
Related issues: +#2307419: AssetCollectionOptimizerInterface should allow listing and deleting all aggregates (optimized collection assets)
FileSize
15.22 KB

#2276219: Asset libraries should declare their license landed! This issue is now unblocked :)

Attached is a straight reroll of #21, but adjusted for #2276219: Asset libraries should declare their license and other changes in HEAD.


#22: there are a few important differences:

  1. Even minified CSS can quite easily be "unminified", the only thing missing then are comments. By its very nature, it cannot be obfuscated (e.g. variable names being renamed to be nonsensical), because CSS selectors and properties need to be interpreted by the browser.
  2. CSS does not contain logic/executable code (except in rare cases "logic" in the form of the calc() property, but that's always trivial). I think that the FSF therefore considers the inability to obfuscate CSS (i.e. you can always access the source code) in combination with the Linking Exception, this continues to meet the GPL's requirements (provided that you also meet, for each linked independent module, the terms and conditions of the license of that module).
  3. For more details, see https://www.gnu.org/philosophy/javascript-trap.html

Also see #1649654: Non-trivial JavaScript files need GPL license declaration for compliant distribution to browsers, #1536810: Adhere to licensing issues when minifying files and #1649670: [META] Improving Drupal sites JavaScript Licence Compliance — as well as other issues where this has been discussed. This has been a known problem for many years, and was discussed by many, it's not just a claim by me!


To address #17, i.e. to fix the reliance on the system.js_optimized_aggregate_files entry in State, which shouldn't be relied upon because it's an overridable implementation detail problem, I propose we expand AssetCollectionOptimizerInterface slightly, to allow for listing and deleting all aggregates. See #2307419: AssetCollectionOptimizerInterface should allow listing and deleting all aggregates (optimized collection assets) for that.

So once again, this issue is blocked, but this should be the last time.

hass’s picture

There is one thing I cannot find in the patch. How can I define that the file is already a minified version? In case a file is already minified it is a waste of cpu to double minify them. I think we need to combine them in such a case only.

Wim Leers’s picture

Great point. I hadn't considered that.

The reason I hadn't considered it yet, is precisely because of what you say: we don't have a flag to indicate that a file is already minified.
Personally, I think that all JS files we ship with should be unminified. Minification should be done by Drupal's JS minifier (which could use UglifyJS or whatever external library is best or carries your preference). Because only then you can easily switch to the unminified version, and hence only then you can easily debug.

In any case, what you're calling out is something that's out of scope for this issue. As it stands, Drupal 8 contrib modules that add a JS minifier would have to GUESS based on heuristics (file name, first 100 bytes, …) whether a JS file is already minified or not. The only reason it's not a problem in HEAD is because we still don't have a JS minifier!
I opened an issue to discuss that: #2307717: Inform Drupal in *.libraries.yml via a new per-JS file "minified" flag whether a JS file has been minified.

hass’s picture

I'm sorry to say, but you are wrong. Check out the assets folder. Ckeditor, html5shiv and others are minified. I also think it's wrong to use UglifyJS in all cases as the maintainers of libs have made extreme tests with several minifiers and decided for the best from their test results, latency, ux and other reasons. If you get this in here without such a "disable" directive we will double minify a lot of code.

I have not seen these minifier code, but minifying 5mb js code on the fly sounds like a crazy task to me. Isn't UglifyJS not a nix only tool, too? This is a showstopper to me.

Wim Leers’s picture

Title: [PP-1] Implement JS Web License Labels to allow for JS minification » Implement JS Web License Labels to allow for JS minification
Issue summary: View changes
Status: Postponed » Needs review
FileSize
16.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,758 pass(es). View
15.03 KB

Hurray, #2307419: AssetCollectionOptimizerInterface should allow listing and deleting all aggregates (optimized collection assets) has landed; which was the last blocker (see #23)! Rerolled the patch.


Already since the reroll in #23, the "only allow gpl-compatible JS files in the aggregate" part of the original patch has been removed. I've now also updated the issue summary to reflect that.


As of #25/#26, we have a new soft blocker, which is: Drupal core doesn't yet allow us to know which files already are minified.

(Yes, hass, you were absolutely right that I'm wrong. CKEditor and others are minified. Of those, I think only CKEditor is rightfully minified, because minifying that is a massive amount of work (because it's a massive amount of JS) and you rarely need to debug it (only when developing CKEditor plugins). html5shiv could also be acceptable because it's extremely, extremely rare that one would need to debug that, plus one doesn't develop against that. But one needs to step through jQuery's code quite frequently, so hence it shouldn't be minified.
I also gave a bad example — Drupal of course wouldn't ship with UglifyJS, because that'd imply a dependency on node.js.
All that being said: without actually having a JS minifier in core, my "should not be minified" reasoning is pointless.)

I think it's a soft blocker, because the double minification really is an existing bug in Drupal core; it already happens in HEAD!

Therefore I think we can continue with this patch just fine now. To fix the double minification problem, I rolled another patch: #2307717-13: Inform Drupal in *.libraries.yml via a new per-JS file "minified" flag whether a JS file has been minified.

nod_’s picture

I'm happy with the patch. Only got a few comments:

Having a (hidden) link at the bottom of the page is not great, especially because I'm guessing people will end up blacklisting /system/jslicense in their robots.txt (because it gives away most of the modules used on the site (security issue too maybe?)).

Having a <link rel="jslicense" href=""> would serve the same purpose. I'm reaching out to the relevant mailing list to know what they'll say.

Other than that the PHP looks good to me, not that I'm a great judge for that :D

nod_’s picture

Status: Needs review » Needs work

Looks like the link stuff instead of the a tag is supported by the librejs firefox extension http://lists.gnu.org/archive/html/help-librejs/2014-02/msg00004.html (and at least unofficially supported). Couldn't get a definite yes/no for now but I say let's just go with <link>

Wim Leers’s picture

Yes, let's do that :) I always disliked the "hidden link at the bottom of the page" approach. Thanks for the feedback!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
16.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch js_license_web_labels-2258313-31.patch. Unable to apply patch. See the log in the details link for more information. View
1.43 KB

Implemented #28/#29.

Wim Leers’s picture

Issue summary: View changes
moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Our way to add [link] tag is ugly but thats not invented here. All feedback has been addressed. PHP code looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll

git ac https://www.drupal.org/files/issues/js_license_web_labels-2258313-31.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 17065  100 17065    0     0   104k      0 --:--:-- --:--:-- --:--:--  130k
error: patch failed: core/modules/system/src/Tests/Common/JavaScriptTest.php:32
error: core/modules/system/src/Tests/Common/JavaScriptTest.php: patch does not apply

The last submitted patch, 31: js_license_web_labels-2258313-31.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
16.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,270 pass(es). View

Straight reroll, with thanks to git's 3-way merge.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. <tr id="core-assets-vendor-ckeditor-ckeditorjs" class="odd">
      <td><a href="http://drupal8alt.dev/core/assets/vendor/ckeditor/ckeditor.js"><code>ckeditor.js</code></a></td>
      <td><a href="https://github.com/ckeditor/ckeditor-dev/blob/887d81ac1824008b690e439a1b29eb4f13b51212/LICENSE.md">GNU-GPL-2.0-or-later</a></td>
      <td class="priority-medium"><a href="http://drupal8alt.dev/core/assets/vendor/ckeditor/ckeditor.js"><code>ckeditor.js</code></a></td>
    </tr>
    

    I'm not sure this row is correct - if I understand https://www.gnu.org/licenses/javascript-labels.html correctly the third row should be a link to a non minified - source code version of the javascript. Maybe the libraries info needs to have a link we can use here.

  2. +++ b/core/lib/Drupal/Core/Asset/JsLicenseWebLabelsAnnotator.php
    @@ -0,0 +1,53 @@
    +      throw new \Exception('Only file JavaScript assets can be optimized.');
    ...
    +      throw new \Exception('Only file JavaScript assets with preprocessing enabled can be optimized.');
    

    Exceptions should not have fullstops and should we be using the general exception class here?

  3. +++ b/core/lib/Drupal/Core/Asset/JsLicenseWebLabelsAnnotator.php
    @@ -0,0 +1,53 @@
    +    // Generate a prefix to be prepended to the "optimized" asset (no actual
    +    // optimizations are made; this is a no-op optimizer) that deep-links to the
    +    // license information on the JavaScript License Web Labels page.
    +    $url = $this->urlGenerator->generateFromRoute('system.javascript_license_web_labels', array(), array('fragment' => drupal_clean_css_identifier($js_asset['data'])));
    +    $prefix = "/** JavaScript asset: " . $js_asset['data'] . '; for license information, see ' . $url  . "  **/\n\n";
    +    return $prefix . file_get_contents($js_asset['data']);
    

    We seem to be lacking test coverage of this.

  4. +++ b/core/modules/system/src/Controller/AssetLicenseInfoController.php
    @@ -0,0 +1,182 @@
    +use Drupal\Component\Utility\String;
    

    Not used

  5. +++ b/core/modules/system/system.routing.yml
    @@ -418,3 +418,11 @@ system.admin_content:
    +system.javascript_license_web_labels:
    +  path: '/system/jslicense'
    

    We have no test that tests this route - afaics.

edit: fix html

Wim Leers’s picture

Status: Needs work » Postponed

#37.1: You're right. That was another reason I originally had "no minified JS at all in core" as one option to fix #2307717: Inform Drupal in *.libraries.yml via a new per-JS file "minified" flag whether a JS file has been minified. But since I was out of this issue and that issue for some time, I must've forgot about that. catch made it clear at https://www.drupal.org/node/2307717#comment-8990911 that we want minified JS in core. So that makes me think the only way to implement JS License Web Labels, is by linking to a CKEditor-hosted version of the source, i.e. by requiring a unminified-source property that links to the unminified source for JS assets that are minified (that have the minified flag set to true).
That means #2307717: Inform Drupal in *.libraries.yml via a new per-JS file "minified" flag whether a JS file has been minified will have to land before I can finish this.

Wim Leers’s picture

lauriii’s picture

Status: Needs work » Needs review
FileSize
15.5 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,641 pass(es), 465 fail(s), and 34 exception(s). View

Rerolled last patch.

Status: Needs review » Needs work

The last submitted patch, 40: js_license_web_labels-2258313-40.patch, failed testing.

tstoeckler’s picture

  1. +++ b/core/lib/Drupal/Core/Asset/JsLicenseWebLabelsAnnotator.php
    @@ -0,0 +1,53 @@
    +      throw new \Exception('Only file JavaScript assets can be optimized.');
    ...
    +      throw new \Exception('Only file JavaScript assets with preprocessing enabled can be optimized.');
    

    Would be slightly more semantic to throw an \InvalidArgumentException here

  2. +++ b/core/modules/system/system.module
    @@ -626,6 +626,12 @@ function system_page_attachments(array &$page) {
    +  // Add the JavaScript Web License Labels link to all pages.
    

    If we don't send any JavaScript at all (i.e. for anonymous users), this shouldn't be added either, right?

YesCT’s picture

Issue tags: -front-end performance +frontend performance

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.

mdrummond’s picture

I just want to confirm a couple of items:

1) Is it now resolved so that all JS can be aggregated in one file regardless of license type?

2) Have we verified that having the link element to the JS license won't result in a file download?

Those seem like two key things for front-end performance with this issue.

Wim Leers’s picture

Version: 8.0.x-dev » 8.1.x-dev

We should be focused on shipping D8 right now, and only working on non-criticals that have long-term consequences for D8. This issue can easily be done in 8.1; the necessary API changes have already been made.

Hence updating the "version" component of this issue.

webchick’s picture

While that is true, I think this is still worth working on in 8.0.x, since:

a) It's an enabler for a performance improvement, and we need more of those. ;)
b) (raised today by nod/jhodgdon) If we try and do JSDoc, without this we're going to bloat the size of JS files unnecessarily.

webchick’s picture

Version: 8.1.x-dev » 8.0.x-dev

Doing that. :)

nod_’s picture

Assigned: Wim Leers » Unassigned

While it'd be cool, let's not expect wimleers to fix all our bugs.

Wim Leers’s picture

:P

lauriii’s picture

Maybe we can clone Wim Leers and then he could fix all the bugs?

lauriii’s picture

Status: Needs work » Needs review
FileSize
16.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,161 pass(es), 808 fail(s), and 119 exception(s). View

Rerolling again

Status: Needs review » Needs work

The last submitted patch, 51: implement_js_web-2258313-51.patch, failed testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DamienMcKenna’s picture

WIP reroll.

The changes to template_preprocess_html() didn't apply as it has changed substantially since 8.0.0-beta11. I've attached one file of the rejected changes to the theme.inc file and another with beta11's template_preprocess_html with the patch applied, so it can be seen in context.

Status: Needs review » Needs work

The last submitted patch, 55: drupal-n2258313-55.patch, failed testing.

Wim Leers’s picture

Thanks for picking this up again!

DamienMcKenna’s picture

Might someone who knows the JS aggregation code be able to take a look at re-applying the missing template_preprocess_html() change? Thanks.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.