Fixing an issue where strategies were applied to incorrect session configurations #489

Merged
merged 1 commit into from May 31, 2012

Projects

None yet

2 participants

@rmarscher

Also added 'configuration' to the filter params so the filters know which session configuration it is processing.

Basically here's what I was trying to do:

Have a default session that I use for normal things. But then have a long lived cookie session adapter that uses an encrypted data strategy.

I found that my default session was getting encrypted too and causing all types of trouble.

This patch fixes that bug (with a test case). I also added 'configuration' to the filter params because without it, my filter had no idea which configuration was being filtered.

I'm using filters to get the methods to only write to my default configuration unless I specifically name a configuration in the options. By default, Lithium writes, clears, deletes, and checks all session adapters (but only reads from the first named session adapter).

@nateabele
Union of RAD member

You know, I actually think I'm gonna completely destroy this API. It's not doing anybody any favors. See the comments on #457. Would that eliminate the need for this?

@rmarscher

Thanks, Nate. I put in my 2 cents over there. Any interest in merging this fix in the short term since it's less controversial? If not, I can probably run off of my fork in the meantime or write my own class for what I need to do with cookies.

@nateabele
Union of RAD member

Wait a sec, can you show me your session configuration?

@rmarscher

I'm on my mobile at the moment. You can check out the test case in my commit. It's modeled on my config. Will post mine up later when I have my laptop.

@nateabele
Union of RAD member

Okay, after review, I'm good with the strategy fix, but not sure about the 'configuration' bit yet. Lemme think on it. Do you wanna open a separate pull request with just the fix?

@rmarscher

Here's what I was actually doing:
https://gist.github.com/2839513

I couldn't figure out the right name for 'configuration'. I can take it out. Its purpose is to inform the filter which configuration it is operating on. I'm not missing some other way to do that, right?

@nateabele
Union of RAD member

Okay, I dig. Yeah, rather than introduce something we'll just be removing later, I think we'll just run with the strategies fix and then when the Session API is fixed, the configuration name will just be a parameter accessible in the filter as normal.

Do you want to open a new pull request, or commit over this one and squash?

@rmarscher

Squashed

@nateabele nateabele merged commit da99ce2 into UnionOfRAD:dev May 31, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment