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
I found out about this issue when trying to add a file with a query string through drupal_add_js(). I needed to do so to have a dynamic .js file (through php) work with my module.
Basically, when attempting to do so, the question mark and ampersand will be escaped to their percentage values in the following function calls
common.inc
-> drupal_get_js()
-> file_create_url()
-> drupal_encode_path()
-> rawurlencode()As this could rename a file to example.js%3Ffoo%3Dbar%26baz%3Dbaw, it can lead to errors because of the following code in common.inc -> drupal_get_js()
$query_string_separator = (strpos($item['data'], '?') !== FALSE) ? '&' : '?';
$js_element['#attributes']['src'] = file_create_url($item['data']) . $query_string_separator . ($item['cache'] ? $query_string : REQUEST_TIME);
This code actually checks for a query string marker (?) and sets a separator based on it, even though there will be no question mark, but %3F, after file_create_url() is called on the URI.
Proposed resolution
I made a patch which correctly separates the already present query string from the file name and reattaches it after the file name has been turned into a URI.
Remaining tasks
Patch needs reviewing (will be supplied in next post)
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | fixed-drupal-get-js-bug-1320996-10.patch | 1.52 KB | kristiaanvandeneynde |
| #8 | fixed-drupal-get-js-bug-1320996-7.patch | 2.19 KB | kristiaanvandeneynde |
| #1 | fixed-drupal-get-js-bug-1320996-1.patch | 2.47 KB | kristiaanvandeneynde |
Comments
Comment #1
kristiaanvandeneyndeAs promised: the patch
Comment #3
kristiaanvandeneynde#1: fixed-drupal-get-js-bug-1320996-1.patch queued for re-testing.
Comment #5
kristiaanvandeneynde#1: fixed-drupal-get-js-bug-1320996-1.patch queued for re-testing.
Comment #7
kristiaanvandeneyndeEDIT: See comment below, changing patch from D7 to D8 to avoid test bot problems (?) and to comply with patch submitting guide.
Comment #8
kristiaanvandeneyndePatched it in Drupal 8 so it can be backported.
Also, the patch in comment 1 contained a logical error in the adjustments to drupal_attributes.
Comment #10
kristiaanvandeneyndeAll changes above to drupal_attributes were unjust.
Ampersands should be escaped in HTML attributes as well.
As a result, the patch now only contains the working fix to drupal_get_js() and can be backported to D7.
Comment #11
kristiaanvandeneyndeComment #12
kscheirer#10: fixed-drupal-get-js-bug-1320996-10.patch queued for re-testing.
Comment #12.0
kscheirerRemoved any reference to drupal_attributes, which was unjustly changed
Comment #15
coredumperror CreditAttribution: coredumperror commentedAny chance that this issue could get some love, please? It completely breaks the ability to include the Google Map Javascript API's libraries with any of the normal javascript inclusion mechanisms. For example, including the maps API with the Drawing library uses a URL like this:
https://maps.googleapis.com/maps/api/js?key=YOUR_API_KEY&libraries=drawing
Since the ampersand gets URL-encoded, both GET args become mangled together.
The only solution I've found is this:
Every other method of adding a script tag to the page (
drupal_add_js(), $form['#attached']) mangles the ampersand.Comment #24
quietone CreditAttribution: quietone as a volunteer commenteddrupal_get_js was removed from Drupal 8.0.x in #2368797: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests.
Therefore, closing as outdated. If this is incorrect reopen the issue, by setting the status to 'Active', and add a comment explaining what still needs to be done.
Thanks!