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

Should $location extract search params from before the #! ? #5964

Closed
bhollis opened this issue Jan 23, 2014 · 11 comments
Closed

Should $location extract search params from before the #! ? #5964

bhollis opened this issue Jan 23, 2014 · 11 comments

Comments

@bhollis
Copy link

bhollis commented Jan 23, 2014

When not in HTML5 mode, given a URL like this:

http://foo.com/?bar=baz

$location.search() produces an empty object, rather than { bar: 'baz' }.

I understand that it's looking only at the part of the URL following #!, but I'm wondering if that's really the correct way to go - it certainly makes it difficult to extract and use those query parameters (without using separate URL parsing code for window.location). Is this just not what $location is for? The docs say:

The $location service parses the URL in the browser address bar (based on the window.location) and makes the URL available to your application." which suggests it should parse the whole URL.

I'd also expect that given a URL like this, not in HTML5 mode:

http://foo.com/?bar=baz#!/?qux=quux

I'd expect $location.search() to be { bar: 'baz', qux: 'quux' } - the merged set of query parameters.

@caitp
Copy link
Contributor

caitp commented Jan 23, 2014

well if search isn't working in hashbang mode when there's no hashbang, that sounds like a bug. care to try and submit a patch? It would be good if you could show a jasmine test which fails without a fix first

@caitp
Copy link
Contributor

caitp commented Jan 23, 2014

Actually, this seems to be from urlResolve.js, where we give an anchor tag the url and get it to parse it for us. So I guess the browser isn't convinced there's a search string either, without the '#/'.... So are you saying that with html5Mode it works as you expect? because it would appear not to

@bhollis
Copy link
Author

bhollis commented Jan 24, 2014

@caitp I'd be happy to write up a PR, but I need to wait for permission to sign the CLA and provide code to the project.

@caitp
Copy link
Contributor

caitp commented Jan 24, 2014

@bhollis it's okay, I've realized that we were not actually sending the urlResolve function the entire relative url, and am submitting a fix for this. It might actually make it to the release after all if we defer until tomorrow.

caitp added a commit to caitp/angular.js that referenced this issue Jan 24, 2014
Before this fix, search queries in hashbang mode were ignored if the hash was not present in the
url. This patch corrects this by ensuring that the search query is available to be parsed by
urlResolve when the hashbang is not present.

Closes angular#5964
caitp added a commit to caitp/angular.js that referenced this issue Jan 24, 2014
Before this fix, search queries in hashbang mode were ignored if the hash was not present in the
url. This patch corrects this by ensuring that the search query is available to be parsed by
urlResolve when the hashbang is not present.

Closes angular#5964
caitp added a commit to caitp/angular.js that referenced this issue Jan 24, 2014
Before this fix, search queries in hashbang mode were ignored if the hash was not present in the
url. This patch corrects this by ensuring that the search query is available to be parsed by
urlResolve when the hashbang is not present.

Closes angular#5964
caitp added a commit to caitp/angular.js that referenced this issue Jan 24, 2014
Before this fix, search queries in hashbang mode were ignored if the hash was not present in the
url. This patch corrects this by ensuring that the search query is available to be parsed by
urlResolve when the hashbang is not present.

Closes angular#5964
@bhollis
Copy link
Author

bhollis commented Jan 25, 2014

Thanks, that's great!

@bhollis
Copy link
Author

bhollis commented Jan 25, 2014

It looks like that fixes the first case, which is great. What do you think about the slightly weirder second example:

http://foo.com/?bar=baz#!/?qux=quux

Where there's both a search string before, and after the hash?

@caitp
Copy link
Contributor

caitp commented Jan 25, 2014

I'm not sure if that's a really valid URL... I'll have to see what people think about it. How are you coming up with a url like that in angular anyway?

@bhollis
Copy link
Author

bhollis commented Jan 25, 2014

That one's theoretical, but I was assuming it was what would happen if I modified $location.search() in non-HTML5-mode after starting with a URL that had a query string.

@caitp
Copy link
Contributor

caitp commented Jan 25, 2014

If that does happen, I think that's a separate bug to look at. But we should really get a reproduction to be sure

caitp added a commit to caitp/angular.js that referenced this issue Jan 31, 2014
Before this fix, search queries in hashbang mode were ignored if the hash was not present in the
url. This patch corrects this by ensuring that the search query is available to be parsed by
urlResolve when the hashbang is not present.

Closes angular#5964
@caitp caitp closed this as completed in cad717b Feb 21, 2014
IgorMinar added a commit that referenced this issue Feb 26, 2014
…bang mode

This reverts commit cad717b.

This change causes regressions in existing code and after closer inspection
I realized that it is trying to fix an issue that is should not be considered
a valid issue.

The location service was designed to work against either "hash" part of the
window.location when in the hashbang mode or full url when in the html5 mode.

This change tries to merge the two modes partially, which is not right. One
reason for this is that the search part of window.location can't be modified
while in the hashbang mode (a browser limitation), so with this change part
of the search object should be immutable and read-only which will only cause
more confusion.

Relates to #5964
@nkomp18
Copy link

nkomp18 commented Sep 1, 2016

Pretty sure this is still broken

@gkalpak
Copy link
Member

gkalpak commented Sep 7, 2016

@nkomp18, this issue is closed. If you think there is still a bug in the latest versions, please open a new bug report (providing the necessary details).

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 a pull request may close this issue.

4 participants