Support from Acquia helps fund testing for Drupal Acquia logo

Comments

spearhead93’s picture

Status: Active » Needs review
FileSize
640 bytes

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

Adding an upgrade path for 'drupal_js_version_query_string'.

spearhead93’s picture

Status: Needs work » Needs review
FileSize
1.32 KB

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

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
FileSize
2.07 KB

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
FileSize
1.32 KB

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.