_drupal_flush_css_js() keeps a history of the last 20 characters used in the query string. These are all different characters. The two latest added characters are appended to the query string.

The function guarantees that the two characters added to the query string change, each time _drupal_flush_css_js() is been called, and that no duplicate two-character strings appear within 20 calls. In more that 20 calls there is a chance of duplicates, though it is rather unlikely, because each new character is chosen at random.

This logic is hard to grasp, because it relies on both a recorded history and randomness. It is too complex IMO.

Other approaches include 1) just generate something “very” random in _drupal_flush_css_js() 2) use a timestamp, or 3) use a counter that is incremented in each call to _drupal_flush_css_js().

This patch implements 2). The timestamp is converted to base 36 for a more compact representation (6 characters). This could be made even more compact, e.g. by reducing the resolution (from 1 second).

CommentFileSizeAuthor
#17 drupal_flush_css_js-D6-2.patch2.4 KBkarschsp
#5 drupal_flush_css_js-D6-1.patch4.03 KBc960657
drupal_flush_css_js-1.patch3.86 KBc960657
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dries’s picture

Dries CreditAttribution: Dries commented

I like this simplification, and I don't really see anything wrong with it.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Looks reasonable - will add a few extra bytes per page. I suppose we could reduce that by dividing the timestamp by something like 100 at the risk that someone will do this operation twice in that time frame.

A simple counter is certainly another option - e.g.

    $old = variable_get('css_js_query_string', '0');
    $new = 1 + intval($old, 36);
   // The counter is converted to base 36 in order to make it more compact.
   variable_set('css_js_query_string', base_convert($new, 10, 36));

so you have to clear the cache 1295 times before you hit 3 characters.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed the patch. I don't think the few extra bytes are a big deal, but we can consider Peter's suggestion in a follow-up patch.

Status: Fixed » Closed (fixed)

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

c960657’s picture

c960657 CreditAttribution: c960657 commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Needs work
FileSize
drupal_flush_css_js-D6-1.patch4.03 KB

D6 backport.

Note that this issue is not only about reducing code complexity but also to avoid that URLs are being reused after 20 cache clears.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal_flush_css_js-D6-1.patch, failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review

#5: drupal_flush_css_js-D6-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal_flush_css_js-D6-1.patch, failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review

There must be some problem with the test bot. The patch applies cleanly here.

Gabriel R.’s picture

Gabriel R. CreditAttribution: Gabriel R. commented

Subscribe.

c960657’s picture

c960657 CreditAttribution: c960657 commented

#5: drupal_flush_css_js-D6-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal_flush_css_js-D6-1.patch, failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review

The last submitted patch, drupal_flush_css_js-D6-1.patch, failed testing.

Eh, what? Over at http://qa.drupal.org/pifr/test/41738 everything is green.

karschsp’s picture

karschsp CreditAttribution: karschsp commented

#5: drupal_flush_css_js-D6-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal_flush_css_js-D6-1.patch, failed testing.

karschsp’s picture

karschsp CreditAttribution: karschsp commented
Status: Needs work » Needs review
FileSize
drupal_flush_css_js-D6-2.patch2.4 KB

Here's an updated version of the patch in #5.

Status: Needs review » Needs work

The last submitted patch, drupal_flush_css_js-D6-2.patch, failed testing.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.