search

jkrall / analytical

Gem for managing multiple analytics services in your rails app.
MIT License
381 stars 93 forks source link

SessionCommandStore broken #48

Closed nirvdrum closed 11 years ago

nirvdrum commented 11 years ago

I've only managed to test on Rails 3.2, but it looks like SessionCommandStore has been broken since 9b0833e86f0a75699fb2f68b85e98267996dcf66 was introduced. The core problem is the config file is now loaded as a HashWithIndifferentAccess, rather than as a simple hash. HashWithIndifferentAccess ensures any hash values are also types of HashWithIndifferentAccess. In order to do so, it must make a new copy of the value. In this case, the value is the session, which is a hash. Creating a new session with indifferent access creates a new hash that is no longer associated with the session, so all modifications are now on an in-memory copy.

nirvdrum commented 11 years ago

And just to confirm, the SessionCommandStore does work fine in 3.0.12. Fortunately this is only broken in master at the moment.

yachi commented 11 years ago

deep_symbolize_keys can do the job, but it depends on rails

nirvdrum commented 11 years ago

@indirect any chance you could look at fixing this before the next analytical release?

indirect commented 11 years ago

@nirvdrum sorry, I don't use analytical anymore, and I'm pretty overwhelmed by Bundler stuff right now :/ feel free to revert my commit until I can get around to fixing the problem.

sfsekaran commented 11 years ago

I'm in favor of reverting the commit for now. Agreed, @nirvdrum?

nirvdrum commented 11 years ago

Sounds good to me.

sfsekaran commented 11 years ago

I'd be reverting the following commits:

08af462 allow Analytical.config without explicit nil arg
88c1b7c reverse merge so that config stays a HWIA (oops)
398bb81 handle empty environment configs
18a4471 allow the config file location to be passed as :config
9b0833e only read the config file once, clean up a bit

Doesn't look like a problem to do so, seeing as they all involve the same file, and they're the latest commits for that file (all by @indirect).

sfsekaran commented 11 years ago

@nirvdrum Let me know if it's still broken and I'll reopen the issue.