-
Notifications
You must be signed in to change notification settings - Fork 325
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
Add "cookiejar" option to config. #180
base: master
Are you sure you want to change the base?
Add "cookiejar" option to config. #180
Conversation
…r file in CWD which it will remove when complete. Added condition for GET requests as well.
Can one of the admins verify this patch? |
@danielatdattrixdotcom Thank you for your contribution - this is great. I have to ask, why did you close the PR? I was planning to test it out and merge once I get out of crunch mode at work and have a little time to review. Cheers, |
Can one of the admins verify this patch? |
@svanoort I ran into an issue it created in not resetting the curl object to a base state so I closed it. The issue presents if you have a PUT then a POST. It needs code to reset the curl object to a ready state without wiping the cookiejar. I'll work out an alternate PR since this one is flawed. |
@danielatdattrixdotcom Probably because it's using CookieJar and CookieFile on the same file, which makes it r/w. One can unset the CookieJar opt to render it read-only, but then you lose the advantage of a persistent session after initial auth. Considering this use case, would it not be better to create an Extractor that gets the cookies (there's already issue #144 to add extractors geared to cookie operations with the next release), and save cookies to a variable? The cookies are already available on the headers (which all extractors get to play with), there's just not a binding to handle them specially yet. |
@svanoort Just posted an updated commit with very minor adjustments. I'm writing tests for a project now, so if I encounter any issues I will adjust as-needed if I find I created a problem. I read through #144 , but since we've got curl and it already has mechanisms for managing cookies I did not see a need to reinvent it and add any more complex configuration setup for the simple use of cookies as a test session. |
…revent a segfault.
@svanoort I ran into a segfault during testing and made one more adjustment. |
@danielatdattrixdotcom I do apologize for the delay (crunch mode at the day job) -- do you consider this to be in a state where I can hit it with the full integrated tests and merge if ready? |
@svanoort Yes, I think so. |
A minor change to use the curl cookiejar and treat one file as a session. I needed this for a Tastypie API using SessionAuthentication. And a white space trimming courtesy of PyCharm.