Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($browser): cookiePath now defaults to '' when basePath is undefined #1910

Closed
wants to merge 4 commits into from
Closed

fix($browser): cookiePath now defaults to '' when basePath is undefined #1910

wants to merge 4 commits into from

Conversation

fredrikbonander
Copy link
Contributor

As described in issue #1191 Safari and IE fails if the cookiePath is undefined.

@IgorMinar
Copy link
Contributor

better fix would be to fix the baseHref method not to return undefined:

return href ? href.replace(/^https?\:\/\/[^\/]*/, '') : href;

should be:

return href ? href.replace(/^https?\:\/\/[^\/]*/, '') : '';

can you also write a test for the $browser.cookies api that will verify that the cookie path is set to empty string by default even if the base element is undefined?

@fredrikbonander
Copy link
Contributor Author

@IgorMinar Sure, it was my first thought but in https://github.com/angular/angular.js/blob/master/test/ng/browserSpecs.js#L558 there's a test verifying that basePath is undefined. If I modify it and remove the test for undefined would this be considered a breaking change?

@IgorMinar
Copy link
Contributor

that test is unnecessarily strict. I think that it's fine to change it to return "" when baseElement doesn't exist

… in Safari and IE

The test verifies that the cookie is set instead of checking that cookie has correct path, this is due to that cookie meta information is not avabile once the cookie is set.
@fredrikbonander
Copy link
Contributor Author

I'm not entirely happy with the test, I would like to test that the correct path is set for a cookie. But since it's not available this might due.

@IgorMinar
Copy link
Contributor

PR Checklist (Minor Bugfix)

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name)
  • PR doesn't introduce new api
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • [-] PR contains e2e tests (if suitable)
  • [-] PR contains documentation update (if suitable)
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@IgorMinar
Copy link
Contributor

landed as 7cb8f8f

thanks!

if you haven't received our t-shirt before and would like one please fill out this form: http://goo.gl/075Sj

@IgorMinar IgorMinar closed this Feb 7, 2013
@IgorMinar
Copy link
Contributor

oops should be 7cb8f8f

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants