This is a sub-task of http://drupal.org/node/1775842 Convert all variables to state and/or config systems.

Files: 
CommentFileSizeAuthor
#16 remove_js_version_string.patch1.32 KBcatch
PASSED: [[SimpleTest]]: [MySQL] 59,320 pass(es).
[ View ]
#14 js_version_settings.patch2.07 KBcatch
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#9 1824848-drupal_js_version_query_string_to_cmi.9.patch1.42 KBmtift
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 1824848-drupal_js_version_query_string_to_cmi.4.patch1.32 KBspearhead93
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#3 1824848-drupal_js_version_query_string_to_cmi.2.patch1.32 KBspearhead93
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#1 1824848-drupal_js_version_query_string_to_cmi.1.patch640 bytesspearhead93
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

spearhead93’s picture

Status:Active» Needs review
StatusFileSize
new640 bytes
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Moving this variable to 'system.js_version_query_string'.

Status:Needs review» Needs work

The last submitted patch, 1824848-drupal_js_version_query_string_to_cmi.1.patch, failed testing.

spearhead93’s picture

StatusFileSize
new1.32 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Adding an upgrade path for 'drupal_js_version_query_string'.

spearhead93’s picture

Status:Needs work» Needs review
StatusFileSize
new1.32 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Moving this one to needs review.

Status:Needs review» Needs work

The last submitted patch, 1824848-drupal_js_version_query_string_to_cmi.4.patch, failed testing.

alexpott’s picture

Status:Needs work» Postponed

This is postponed on #1798738: Move css_js_query_string to state system and #1809206: KeyValueFactory hard-codes DatabaseStorage as this change needs the keyvalue service to work early on in update.php ie. before the state table actually exists.

Damien Tournoud’s picture

Title:Convert drupal_js_version_query_string to CMI system» Convert drupal_js_version_query_string to the state system
sun’s picture

Component:configuration system» theme system

Not sure whether this belongs into theme system or base system. But definitely not configuration system. :)

mtift’s picture

StatusFileSize
new1.42 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Just a re-roll

mtift’s picture

Assigned:spearhead93» Unassigned
Status:Postponed» Needs review

I'm just curious if this patch will pass

Status:Needs review» Needs work

The last submitted patch, 1824848-drupal_js_version_query_string_to_cmi.9.patch, failed testing.

mtift’s picture

Status:Needs work» Postponed
catch’s picture

Priority:Normal» Critical
catch’s picture

Title:Convert drupal_js_version_query_string to the state system» Convert drupal_js_version_query_string to settings
Status:Postponed» Needs review
StatusFileSize
new2.07 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Untested patch but this might be enough.

This never gets set anywhere, it's just a get-out in case there's a query string conflict.

However I'm also having a hard time seeing why this is needed at all - the query string on the file being loaded itself doesn't make any difference to what the js sees on the page. So I'll upload another patch to remove this altogether...

Status:Needs review» Needs work

The last submitted patch, js_version_settings.patch, failed testing.

catch’s picture

Title:Convert drupal_js_version_query_string to settings» Remove drupal_js_version_string variable (instead of converting it)
Status:Needs work» Needs review
StatusFileSize
new1.32 KB
PASSED: [[SimpleTest]]: [MySQL] 59,320 pass(es).
[ View ]

Here's the patch to just remove it altogether. Re-titling again.

chx’s picture

Status:Needs review» Reviewed & tested by the community

Note that this only changes the possibility to override the v in $_GET['v'] in case some boneheaded JS library is hardwired to look for that. It's 2013. Don't use boneheaded JS libraries!

Edit: Note that this only changes the possibility to override the v in $_GET['v'] for the source URL of the JS, not even the page. Why that needs to be overridable is an absolute mystery. Maybe some truly stupid CDN? Can't imagine a real world scenario.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

The original rationale for this appears to be #721400-59: Order JS files according to weight, don't change filenames for aggregated JS/CSS:

"we can't assume that every JS file/library in existence will gracefully accept ?v=X in its URL and we should allow a way for a site to change that behavior to some other query parameter that won't conflict."

Talked about this more with chx in IRC, and he pointed out that the JS aggregation feature itself already causes "if your JS makes assumptions about the URL the source file comes from, you're screwed." So this seems like it was a bogus use case to begin with. Also ran this past nod_ and he said it was good. Yay for one fewer variable! :)

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.