Navigation Menu

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

Jump to bottom

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

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

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

diosmosis opened this issue Jul 30, 2015 · 4 comments
Assignees
@tsteur
Labels
Bug For errors / faults / flaws / inconsistencies etc.
Milestone
3.0.0-b1

Comments

@diosmosis
Copy link
Member

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).
@diosmosis diosmosis added the Bug For errors / faults / flaws / inconsistencies etc. label Jul 30, 2015
@diosmosis diosmosis added this to the 3.0.0 milestone Jul 30, 2015
@tsteur tsteur mentioned this issue Jul 30, 2015
Merged
@tsteur
Copy link
Member

tsteur 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
Copy link
Member Author

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
Copy link
Member

tsteur 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
Copy link
Member

tsteur commented Aug 20, 2015

fixed via #8442

@tsteur tsteur closed this as completed Aug 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tsteur tsteur

Labels
Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Milestone
3.0.0-b1
Development

No branches or pull requests
3 participants
@diosmosis @tsteur @mattab