JavaScript requiring a query string cannot be loaded

+1

Arbitrary query strings can also break certain JavaScripts, for example TinyMCE, as mentioned in http://drupal.org/node/218296#comment-810963

The workaround for now is to set the $cache parameter to FALSE -- however, this results in browsers not caching the JavaScript at all.

So, in addition to those patches here, we badly need a way to force no query string at all for certain drupal_add_js() calls.

Marking as critical.

I also came across this issue in Drupal 6 - see #242875: No way to disable 'cachebuster' query-string appended to CSS file links.. Have marked that as a duplicate of this one.

This issue also breaks language pack loading for the tinyMCE module. The Language packs javascript url returns a drupal 404 error, thus the javascript is never loaded. This should be patched for drupal 6.x as well.

Bugs are fixed in HEAD first.

Reroll for D7.

For D7, I'd suggest to allow developers to pass $options = array('query' => FALSE) in the new $options array to disable the query string for certain JS files only.

Also, this issue unfortunately seems to deal with two issues:

a) Original poster: Drupal's query string is not properly added to an existing query string, i.e. myfile.js?arg1=value?B. There are two ? in the resulting URL. The original poster needs a way to properly assign a query string, f.e.

drupal_add_js($filename, array('query' => 'arg1=value'));

b) Drupal's query string breaks the execution of certain scripts, i.e. myfile.js?B, is sufficient to break the execution of certain scripts (perhaps in specific browsers only, dunno). Since setting the 'cache' option to FALSE results in yet another query string ($info['cache'] ? $query_string : '?' . REQUEST_TIME), there should be an option for developers to disable Drupal's query cache string for certain scripts only, f.e.

drupal_add_js($filename, array('query cache' => FALSE));

I seem to have the same problem. I'm new to drupal and all the lingo. Here's a screen shot that explains what I get when I try to upload an image using Tinymce and the wysiwygAPI. I also had the same result when trying to get Image assist to work without the error messages displayed.
Also I've managed to get simple patches working before but I'm having trouble understanding which lines to add or remove on this one.

Bugs are fixed in the current development branch first.

@sodafox: Also, please do not change issue classification. The bug you are experiencing seems to be limited to using Wysiwyg API and Image Assist. If you were experiencing the bug of *this* issue, neither TinyMCE nor Image Assist would show up at all. Please file a new issue in Image Assist's queue (but search for existing issues first). Thanks.

Better title. Point 2) of my last summary was already fixed elsewhere.

Re-rolled to keep up with HEAD.

I concur - this is not useful at all.
First it's obfuscated, most developers who

* write file.css
* add file.css to theme.info

Expect file.css to be called from the browser - not file.css?[a-z]

Secondly this has silently bust our mod_deflate in Apache for all CSS's served - meaning 10x the bandwidth and latency implicit in downloading a larger file.

Third, content switching above Drupal hosts to pull css/media from a.another server is now bust - all content is being served from our dynamic host. I simply cannot configure our Layer5 Load balancers to manage a ? in the GET to determine which server to get the data from.

How do I switch this madness off.

- Cres

I think you misunderstand this thread. It's not to add the ability to turn off query parameters completely, but to fix a bug when an included file already includes a query parameter. If you want to add an option to disable that functionality completely, I suggest you create a new issue.

#11: 243251_query_string.patch queued for re-testing.

RTBC if bot passes, please.

Based on @sun above

Actually, I was mistaken and meant another patch in the queue, but this one looks good, too. ;)

We should get some tests for this. This is the very definition of something we will easily break in the future trying to "optimize" this part of the code and not notice when the regression is introduced.

I wrote a small test that checks that css and js files with an appended querystring will display correctly in the inclusions.

When I ran the test I saw that locale got upset with urls that have an appended query string when it is translating js files, so I fixed that.

I also had to do some alterations to the patch because of #228818: IE: Stylesheets ignored after 31 link/style tags. I had to put a check_plain() on the file inclusion on @import css rules. I am not sure that is cool, but it works. I did some research on whether @import should display urls escaped or not. I did not really find anything, but it is considered good practice that link tags escapes urls, so I figured the same was true for @import.

+++ includes/common.inc
@@ -3037,7 +3037,8 @@ function drupal_pre_render_styles($elements) {
+            $qs_sep = (strpos($item['data'], '?') !== FALSE) ? '&' : '?';
+            $import[] = '@import url("' . check_plain(file_create_url($item['data']) . $qs_sep . $query_string) . '");';

@@ -3058,7 +3059,8 @@ function drupal_pre_render_styles($elements) {
+            $qs_sep = (strpos($item['data'], '?') !== FALSE) ? '&' : '?';
+            $element['#attributes']['href'] = check_plain(file_create_url($item['data']) . $qs_sep . $query_string);

@@ -3674,7 +3676,8 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
+          $qs_sep = (strpos($item['data'], '?') !== FALSE) ? '&' : '?';
+          $js_element['#attributes']['src'] = file_create_url($item['data']) . $qs_sep . ($item['cache'] ? $query_string : REQUEST_TIME);

$qs_sep still needs a proper variable name.

+++ modules/simpletest/tests/common.test
@@ -682,6 +682,16 @@ class CascadingStylesheetsTestCase extends DrupalWebTestCase {
+   * Tests that the query string remains intact when adding css files that have
...
+    $this->assertRaw(drupal_get_path('module', 'node') . '/node.css?arg1=value1&arg2=value2&' . $query_string, t('Query string was appended correctly to css.'));

In comments and messages, CSS should always be uppercase.

+++ modules/simpletest/tests/common.test
@@ -1255,6 +1265,16 @@ class JavaScriptTestCase extends DrupalWebTestCase {
+   * Tests that the query string remains intact when adding js files that have
...
+    $this->assertRaw(drupal_get_path('module', 'node') . '/node.js?arg1=value1&arg2=value2&' . $query_string, t('Query string was appended correctly to js.'));

Ditto for JS. Additionally, we don't abbreviate JavaScript (note the special casing)

+++ modules/simpletest/tests/common_test.module
@@ -183,3 +189,12 @@ function common_test_library() {
+ * Adds a js file and a css file with a query string appended.

Ditto.

+++ modules/simpletest/tests/common_test.module
@@ -183,3 +189,12 @@ function common_test_library() {
+   return 'Drupal is awesome';

Unless I overlooked something, this string is not used by tests, so we should just return an empty string here.

139 critical left. Go review some!

Reroll with changes from review :)

+++ includes/common.inc
@@ -3037,7 +3037,8 @@ function drupal_pre_render_styles($elements) {
+            $import[] = '@import url("' . check_plain(file_create_url($item['data']) . $query_string_separator . $query_string) . '");';

@@ -3058,7 +3059,8 @@ function drupal_pre_render_styles($elements) {
+            $element['#attributes']['href'] = check_plain(file_create_url($item['data']) . $query_string_separator . $query_string);

@@ -3674,7 +3676,8 @@ function drupal_get_js($scope = 'header', $javascript = NULL) {
+          $js_element['#attributes']['src'] = file_create_url($item['data']) . $query_string_separator . ($item['cache'] ? $query_string : REQUEST_TIME);

1 of 3 doesn't use check_plain() though - any not-so-obvious reason for that?

139 critical left. Go review some!

You are right, it is not so obvious...
The js one that does not use check_plain is drupal_get_js(). That function uses theme_html_tag, that calls drupal_attributes and that runs check_plain.

I wonder if it is OK that it is the responsibility of the theming layer to use check_plain?

More precisely, it seems that this check_plain() is unnecessary, and will lead to double encoding:

$element['#attributes']['href'] = check_plain(file_create_url($item['data']) . $query_string_separator . $query_string);

That is so true. I removed the checkplain on the link that is going in for theming with theme_html_tag

Looks good to me.

Committed to CVS HEAD. Thanks.

Based on Dries' comment above...

probably needs to be backported.

@creslin

Secondly this has silently bust our mod_deflate in Apache for all CSS's served - meaning 10x the bandwidth and latency implicit in downloading a larger file.

That's a configuration problem on your end:

[straussd@web3 public_html]$ cat .htaccess 
<FilesMatch "\.css$">
  SetOutputFilter DEFLATE
  Header set X-Ping "Pong"
</FilesMatch>
[straussd@web3 public_html]$ cat icanhasdeflate.css 
Aaaaaaaaaah

http://straussd.fourkitchens.com/icanhasdeflate.css?nocanhas=1

GET /icanhasdeflate.css?nocanhas=1 HTTP/1.1
Host: straussd.fourkitchens.com
User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.8) Gecko/20100202 Firefox/3.5.8
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive

HTTP/1.1 200 OK
Date: Fri, 05 Mar 2010 13:55:58 GMT
Server: Apache/2.2.3 (Red Hat)
Last-Modified: Fri, 05 Mar 2010 13:54:07 GMT
Etag: "be0cd5-c-4810e0c8dfdc0"
Accept-Ranges: bytes
Vary: Accept-Encoding
Content-Encoding: gzip
X-Ping: Pong
Content-Length: 25
Connection: close
Content-Type: text/css
----------------------------------------------------------

Third, content switching above Drupal hosts to pull css/media from a.another server is now bust - all content is being served from our dynamic host. I simply cannot configure our Layer5 Load balancers to manage a ? in the GET to determine which server to get the data from.

This also sounds like a configuration issue.

This might help someone attempting to use Google Closure in Drupal. I'm still on Drupal 6.14. I was trying to use drupal_add_js to source base.js from Google's closure library. Like all other such calls to include JS, Drupal appended "?s" to the filename (i.e.,
/base.js?s). The result was that none of my goog.require() statements were working. Any code referencing google closure library vars resulted in undefined. Basically, base.js seemed to NOT be downloaded, and so goog namespace was unknown and then all goog.require() calls failed. Only when I added the lines shown below directly in head of page.tpl.php (sans the "?s" appended to the filename), and quit using drupal_add_js() for closure JS files, did closure start working and all the undefined errors went away.

With my page.tpl.php customized like this, my desired google closure code started working:

<head>
...
  <script src="/drupal-6.14/misc/closure/goog/base.js"></script>
  <script>goog.require('goog.json');</script>
  <script>goog.require('goog.dom');</script>
  <script>goog.require('goog.ui.TableSorter');</script>
  <?php print $scripts; ?>
</head>

Is there any interest in this issue? Has this issue been fixed in the latest D6?

I don't think this will be backported.

This is still broken for drupal_add_js with type "file".
file_create_url() url encodes the javascript item's "data" property, breaking the url if it includes a query string.
Works fine for "external" javascripts.

The test included in the patch above was simplified to remove the js's query string by #829822-25: Check Drupal 7 core for vulnerabilities in SA-CONTRIB-2010-066, in 2d3af8fe.