For #1805444-19: garbageCollection followups to TempStore.
Patch attached that replaces one arbitrary number with another arbirtary number.

Alternate solution:
https://twitter.com/xjmdrupal/status/254706375014379523

CommentFileSizeAuthor
booorrrring.patch3.24 KBxjm

Comments

Tor Arne Thune’s picture

Tor Arne Thune commented

There is a problem with the number 299792458? If not, I agree that a Drupal constant would make the code more explicative, as to why this number is used. The other approach is just changing an arbitrary number for another one, no?

chx’s picture

chx commented
Status: Needs review » Needs work

Drupal loves fun. Tests are excellent places for bananas, bunnies and speed of light. In this case, however, adding a comment what it is a justified request because the default expire time is also something most people won't grasp immediately (in high school we needed to know only the first 20 powers of 2, and only the first 10 for 3 and 5 and I never learned really anything after high school :P so even I might not grasp it's 2**31 at first sight) and they might think this has to do something with it.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Drupal hates both science and fun :P » Use a more easily recognized arbitrary integer in k/v expiration tests :)
Status: Needs work » Needs review

Retitling to friendliness. :P

Let's just commit this, okay? No need to make a big deal. Besides, the dimensional analysis shows the value is problematic anyways, since it occurs to me now we are actually supplying a value for seconds, and the integer above is meaningful as a value of meters per second. :)

Tor Arne Thune’s picture

Tor Arne Thune commented

Not sure why the new number is more easily recognizable as being arbitrary, but if someone doesn't like the other number, go for it :)

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Ahem. (whistles innocently and looks the other direction)

xjm’s picture

xjm
she/her
Primary language English
commented

@dawehner points out another reason this is probably a reasonable fix:
https://twitter.com/da_wehner/status/256013043434131457

Since we need a decimal integer and all, we might as well use a simple-looking one. Short of rewriting Drupal to use a base-e or base-pi number system... ;)

lars toomre’s picture

lars toomre commented

Thanks @xjm. As I stated in the original issue, I had no clue about where the number 299792458 came from. This ichange is much more reasonable. Hence, +1 RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Thank you xjm. :) I'm sorry for my ignorance. In my defense, I went to public school in the US, so. ;)

Committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.