JS and CSS aggregation deletes license information

Marking as non-critical.

How would you identify a copyright comment? It is many times not in the @copyright form you use here. Look at the jquery comment for example.

How the copyright should look like is not specified by us, but the copyright holder, as in the case of the jquery copyright for example.

Either we just include the first phpdoc comment of all files in the aggregated version, or we define some custom marker, like [copyright], [/copyright], which the aggregation function then collects into a new comment at the beginning of the aggregated file. The latter is "more secure", of course, but would require to add these copyright tags to all copyrighted files, e.g. to jquery.js.

What we are required to keep and what we are able to modify depends on the exact license text, not some gut feelings. As long as it is not strengthened by a license quote, it is not possible to modify the exact copyright text used by the originating authors.

OK, seems like I was not understandable. The issue comes down to this: does the GPL says that we should keep the original copyright notice as-is, and we should not modify it? Does it say that we can modify it if we want? Does it say nothing about this topic? (Drupal only allows GPL contributions).

From Drupal's LICENSE.txt:

  1. You may copy and distribute verbatim copies of the Program's
source code as you receive it, in any medium, provided that you
conspicuously and appropriately publish on each copy an appropriate
copyright notice and disclaimer of warranty; keep intact all the
notices that refer to this License and to the absence of any warranty;
and give any other recipients of the Program a copy of this License
along with the Program.

www.gnu.org has a FAQ (http://www.gnu.org/copyleft/gpl.html) that -- though I didn't see this specific question -- addresses similar issues. In it, the following notices are recommended:

To do so, attach the following notices to the program. It is safest to attach them to the start of each source file to most effectively state the exclusion of warranty; and each file should have at least the “copyright” line and a pointer to where the full notice is found.

    <one line to give the program's name and a brief idea of what it does.>
    Copyright (C) <year>  <name of author>

    This program is free software: you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
    the Free Software Foundation, either version 3 of the License, or
    (at your option) any later version.

    This program is distributed in the hope that it will be useful,
    but WITHOUT ANY WARRANTY; without even the implied warranty of
    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
    GNU General Public License for more details.

    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.

This sentence, however, probably accurately describes the bare minimum of information necessary on each file:

It is safest to attach them to the start of each source file to most effectively state the exclusion of warranty; and each file should have at least the “copyright” line and a pointer to where the full notice is found.

This "barest minimum" is more or less what seems to be found on jquery.js:

/*
 * jQuery 1.1.3 - New Wave Javascript
 *
 * Copyright (c) 2007 John Resig (jquery.com)
 * Dual licensed under the MIT (MIT-LICENSE.txt)
 * and GPL (GPL-LICENSE.txt) licenses.
 *
 * $Date: 2007-07-01 08:54:38 -0400 (Sun, 01 Jul 2007) $
 * $Rev: 2200 $
 */

All that said, of course, bear in mind that I am not a lawyer, nor do I play one on tv.

This is not only about GPL, though. A custom theme might have a custom copyright notice (as e.g. bluebeach on drupal.org) that one would not want to lose due to compression. So, I think that having some tags in the original css/js files that the aggregator parses out would be the only way to accomplish this. The aggregator should then include the content from between these tags in a comment at the top of the aggregated stylesheet/script file.

How about this: we list the aggregated JS/CSS files in the aggregated file itself, thereby mentioning that for specifics about licenses, you should look in those files (which are simply the originals).

The point made by Wim is good, anybody that knows the GPL (and other licenses) that know if that is a good enough solution?

@hass: isn't that what my sugestion does? "linking" to the location of the license?

+1 for Wim's idea

Here is a patch implementing what Wim suggested.

Please have a look on Drupal Coding Standards.

In short:

• Rename $fileHeader to $file_header
• Fix string concatenation and do not use double quotes unless needed
  • Example: Change " * ".$GLOBALS['base_url']."/".$file."\n"; to ' * '. $GLOBALS['base_url'] .'/'. $file ."\n";

Ok, here we go again. :)

There are still wrong string concatenations and malformed indents:

+          // Add the url to the aggregated file.
+          $file_header .= ' * '.$GLOBALS['base_url'].'/'.$file."\n";
{...}
-    file_save_data($data, $csspath .'/'. $filename, FILE_EXISTS_REPLACE);
+    file_save_data($file_header.$data, $csspath .'/'. $filename, FILE_EXISTS_REPLACE);
{...}
-        $contents .= _drupal_compress_js(file_get_contents($path). ';');
+        $contents   .= _drupal_compress_js(file_get_contents($path). ';');

btw: String concatenation after file_get_contents($path) seemed to be wrong before and may be fixed herein, too.

Comments should always end with a dot.

+    // Close the comment that list the aggregated files

In the cases where there are a line break in the string, what is the Drupal way of doing it?

$file_header .= " * The below content is aggregated from the following files.\n";

or

$file_header .= ' * The below content is aggregated from the following files.'."\n";

You can use double quotes here.
However, if you'd use single quotes, string concatenation should look like '...following files.' . "\n"; , meaning: insert a space in front of and behind the concatenation character if the preceding and following term is a string.

Sorry for my ignorance.
I have read about string concatenations in the Drupal Coding Standards 4 times now, and I think i got it. The first time i read it, I was rather tired. :)
The Coding Standards says to place a space after the dot if the following term is not a quote. I see "\n" as a quote, and therefor there is no space before it. If this applies to only single-quote quotes, there is a need for two more spaces in the patch.

The rest of the problems, I think I have managed to fix.

Looks great - however, it seems you accidently inserted white-space after

-      if ($info['preprocess']) {
+      if ($info['preprocess']) {        

After this quick fix, change the status to patch needs review, please.

I'm sorry, it seems we overlooked these accidentally changes:

-  $data = '';
-
+  $data = '';  
+ 
{...}
-function drupal_build_js_cache($files, $filename) {
-  $contents = '';
-
+function drupal_build_js_cache($files, $filename) {  
+  $contents   = '';  
+  

However, patch applies correctly. Works as described. Changed a CSS file in the queue and reference list was successfully updated.

One more review and this is RTBC.

Fixed, along with some other small things.

Here is a patch for Drupal 5.

we distribute drupal with license information. i don't think we are obligated to put a license into every page we serve.

needs a re-roll.

Is this still a issue, or something that should not be fixed?

Reroll against HEAD.

It seems that this bug is no longer active : without this patch, license is stored with optimize mode enabled :

http://localhost/drupal/7.x/sites/default/files/js/ba699c2d833da30af428e... :

/*
 * jQuery 1.2.6 - New Wave Javascript
 *
 * Copyright (c) 2008 John Resig (jquery.com)
 * Dual licensed under the MIT (MIT-LICENSE.txt)
 * and GPL (GPL-LICENSE.txt) licenses.
 *
 * $Date: 2008/06/12 19:49:10 $
 * $Rev: 5685 $
 */

Someone can confirm ?

Mark as postponed until #119441: Compress JS aggregation reopen.

So it's good for me :

/**
 * The below content is aggregated from the following files.
 * For licensing information please look in the original files.
 *
 * http://localhost/drupal/7.x/modules/node/node.css
 * http://localhost/drupal/7.x/modules/system/defaults.css
 * http://localhost/drupal/7.x/modules/system/system.css
 * http://localhost/drupal/7.x/modules/system/system-menus.css
 * http://localhost/drupal/7.x/modules/user/user.css
 * http://localhost/drupal/7.x/themes/garland/style.css
 */
.node-unpublished{background-color:#fff4f4;}.preview .node{background-color:#ffffea;} ...

Only feature requests and tasks can be assigned to D8 for now, because the Drupal 8 tree is not opened yet.

Re-roll to HEAD, just minor changes.

Ok, I need some e help here. Any ideas how to get this patch to not fail the color module tests. I guess it's the header in the CSS files that breaks the test.

#50 is a bad patch. #41 is in the right format but in the latter there must have been some typo.

@xqus - Any idea what the minor changes were?

I'll check this out.

A patch that fixes this problem lives over at #2258313: Add license information to aggregated assets.