Added possibility to define cookie path parameter #2 #1201
Conversation
addon/session-stores/cookie.js
Outdated
| @@ -148,6 +161,8 @@ export default BaseStore.extend({ | |||
| init() { | |||
| this._super(...arguments); | |||
|
|
|||
| this.set('_cookiePath', Configuration.baseURL); | |||
There was a problem hiding this comment.
This should only be set when it is null (as it might have been set when the cookie store instance was created with Cookie.create({ cookiePath: '/whatever' }) and shouldn't be overridden in this case).
|
|
||
| const { assign: emberAssign, merge, run } = Ember; | ||
| const assign = emberAssign || merge; | ||
|
|
||
| describe('AdaptiveStore', () => { | ||
| let store; | ||
|
|
||
| beforeEach(function() { | ||
| Configuration.load({ baseURL: '/' }); |
There was a problem hiding this comment.
tests in cookie-store-behaviour.js expect that cookie path is set to '/' e.g.
https://github.com/simplabs/ember-simple-auth/blob/master/tests/unit/session-stores/shared/cookie-store-behavior.js#L110
I guess there are 2 ways to sort this out - either change it in all asserts from path === '/' to path === Configuration.baseURL or just set a default baseURL for all tests (which I did)
There was a problem hiding this comment.
I see. I'd just set the cookiePath property on the created store instance instead (here) in these cases so the test is independent of the Configuration.
There was a problem hiding this comment.
yes, that's probably a better option then to globally force this setting to all tests
| @@ -17,6 +18,7 @@ describe('CookieStore', () => { | |||
|
|
|||
| beforeEach(function() { | |||
| store = createCookieStore(FakeCookieService.create()); | |||
| Configuration.load({ baseURL: '/' }); | |||
IgorKvasn
force-pushed
the
cookie-path2
branch
from
d4b5f64 to
46829ae
Compare
|
hmm there are some regression tests failing (at least that's how I understood Travis) - I guess to make it backwards compatible, cookie path should be |
|
nope, fastboot tests are failing |
You are right - defaulting to |
|
@IgorKvasn: can you rebase on |
marcoow
force-pushed
the
cookie-path2
branch
from
2d5b4c8 to
a0f2b7e
Compare
marcoow
force-pushed
the
cookie-path2
branch
from
a0f2b7e to
2118584
Compare
| @@ -218,7 +229,7 @@ export default BaseStore.extend({ | |||
| let cookieOptions = { | |||
| domain: this.get('cookieDomain'), | |||
| expires: isEmpty(expiration) ? null : new Date(expiration), | |||
| path: '/', | |||
| path: this.get('cookiePath'), | |||
There was a problem hiding this comment.
Hm, this is actually a breaking change. We'll need to default the property to '/' instead (as opposed to what I said above this was already the default previously and that obviously has to stay that way).
marcoow
force-pushed
the
cookie-path2
branch
from
2118584 to
b059dba
Compare
marcoow
force-pushed
the
cookie-path2
branch
from
b059dba to
f2fc829
Compare
replacement for a bad PR #1196