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

Hash in Piwik frontend URLs should include ? instead of just / #8462

Closed
diosmosis opened this Issue Jul 30, 2015 · 4 comments

Comments

Projects
None yet
3 participants
@diosmosis
Copy link
Member

commented Jul 30, 2015

Currently the UI frontend will structure URLs like this:

http://localhost/?module=CoreHome&action=index#/module=Goals&action=index

This is ok for now since we don't use angular for URL parsing, however it won't be helpful later on. Using $location.search() with such a URL will return an empty object, since there is no '?' in the hash. Angular treats the string as a path instead of a query string.

URLs should look like this:

http://localhost/?module=CoreHome&action=index#/?module=Goals&action=index

Changing this may cause problems w/ the historyService, would need to test (though UI tests should cover it).

@tsteur

This comment has been minimized.

Copy link
Member

commented Jul 30, 2015

FYI: This is already implemented in #8442 for same reason.

It will be ...#?...

@tsteur tsteur self-assigned this Jul 30, 2015

@mattab mattab modified the milestones: 3.0.0, 3.0.0-b1 Jul 30, 2015

@diosmosis

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2015

Can this be put into 2.15? It would be useful for a plugin I'm working on.

I assume BC for URLs might be an issue, right? I think the history service might take care of that automatically, but not sure.

@tsteur

This comment has been minimized.

Copy link
Member

commented Jul 31, 2015

BC for URLs should not be an issue for the Piwik reporting UI itself (for exported widgets etc it is an issue but this one doesn't affect them).

The PR cannot be put into 2.15, there are several breaking API changes. There were also various problems with those URL changes starting from UI tests to overlay to ... and there were code changes needed in various places. I reckon it would be easier to work with broadcast for now. I had to create a service to access $location.search() anyway (see https://github.com/piwik/piwik/pull/8442/files#diff-a9b4a06fd105d04eaf7fb97abade0dd4R1) which might accesses broadcast again as there are some issues with $location.search:

Eg when having period=day&period=year angular would otherwise return ['day', 'year'] instead of 'year'

and

$location.search() returns empty when query params are not preceded by hash see angular/angular.js#7239

What does it mean? When you have a URL like index.php?module=action&action=test you cannot get the value with $location.search('module'). We have a lot of such URLs... it should be index.php?module=action&action=test as well. We could change this behaviour I think be enabling HTML5 URL mode but then we would always have to test things with a browser that supports HTML 5 mode and with a browser that doesn't. I'm not exactly sure which browser support HTML 5 mode but IE 8 doesn't which should be still supported in 2.15.

@tsteur

This comment has been minimized.

Copy link
Member

commented Aug 20, 2015

fixed via #8442

@tsteur tsteur closed this Aug 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.