ActiveSupport::TimeZone.[] incorrectly stores bogus values in zones_map

I've included a test that passes in master... I just stubbed out the lookup method to return nil, to ensure that we're not explicitly relying on the TZInfo to do the right thing. And I don't think that storing nil values in @zones_map is the intended behavior. If it is, then it was probably done without understanding how it can break other time zone functionality.

For a little background, our app had some calendar events with time zones which had been accidentally sanitized, so the ampersands were HTML encoded. The current TZInfo behavior is correct, and if it was included in Rails 2.3.8, an exception would've been raised as soon as we tried to use the bad time zone. Unfortunately, what actually happened was that some of our backends fielded these calendar event requests as soon as they started up, and nil values "poisoned" the @zones_map. The next time someone tried to draw a time zone menu, we got an exception while trying to sort @zones_map. As you can imagine, this was a really difficult issue to track down, and it required that we roll back to 2.3.5 until the problem was sorted out.

FWIW, I think it's a great idea to update the tzinfo dependency to 0.3.22, but only because it gives us the latest tzdata. I'm pretty sure this behavior is the same in 0.3.16.

(from [80473e035a166d270a204aedaf01c2b80415b8e4]) test that unknown zones don't store mapping keys

[#4942 ActiveSupport::TimeZone.[] incorrectly stores bogus values in zones_map]

Signed-off-by: Santiago Pastorino santiago@wyeworks.com
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/80473e035a166d270a204aedaf01c2...

Hey guys,

The 2.3 backport patch causes some additional unexpected behavior. It will never use the stored value in zones_map, and create a new TimeZone object each time #[] is invoked. Here's a patch to fix it.

Could you please include tests in the patch? Thanks!

Leigh your patch is ok but it's a performance improvement.

-            if tz = lookup(arg)
-              zones_map[arg] ||= tz

From this code you're removing note the ||=, if zones_map[arg] have something it will be returned before the assignation to tz. But i agree we can check zones_map[arg] before to not do some unneded lookups.

So José that doesn't need a test, all the things we need to test are already tested.

Here's a patch with both the lookup fix and a test. I couldn't think of any way other than mocking out the call to find_tzinfo seeing as object apparently doesn't get stored anywhere. If someone else has any better ideas, I'd be glad to hear them.

Does this solve this problem: http://stackoverflow.com/questions/3027704/nomethoderror-for-time-z... ?

@plind yes, sorry I didn't see this for a month. This resolves the issue.