Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from May 31, 2012

Conversation

rmarscher
Copy link
Contributor

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Member

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

@rmarscher
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

Squashed

nateabele added a commit that referenced this pull request May 31, 2012
Fixing an issue where strategies were applied to incorrect session configurations
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants