search

brendanheywood / moodle-local_cleanurls

Lets drag Moodle's url structure into this century...
36 stars 24 forks source link

Replace state not keeping some query params #88

Closed brendanheywood closed 6 years ago

brendanheywood commented 7 years ago

As an example if I go to:

https://s-foo.catalyst-au.net/local/envbar/?PROFILEME

then when it loads internally it has cleaned the /index.php off the end which is fine, but then it also does this:

<script>history.replaceState && history.replaceState({}, '', 'https://s-foo.catalyst-au.net/local/envbar/');</script>

This isn't correct as it has eaten the query params. I suspect this is a fairly widespread problem. It's not a problem for this apge load, but if you were to reload the page it might not be a valid url at all, eg missing a ?id=123 or whatever. This would also mess up any analytics code too.

brendanheywood commented 7 years ago

We need to be a little discerning here, as it's possible we may have a url like:

/course/blah.php?param1=foo

which gets cleaned into

/course/blah/foo

but then we may also have

/course/blah.php?param1=foo&param2=bar

which should either not be cleaned (depending on the url and the param) or it should pass through like:

/course/blah/foo?param2=bar

There are some params like ?sesskey which we want to always ignore out for the purposes of matching but need to make sure they are retained.

roperto commented 6 years ago

Regarding /course/blah.php?param1=foo&param2=bar should go to /course/blah/foo?param2=bar this is already working for URLs.

From my tests, the problem is when you hit a page using the "UNCLEAN" then the cleaner tries to replace the address bar with its clean version (it does not go through the router as the address was valid at first). I am still investigating why.

roperto commented 6 years ago

@brendanheywood From what I can see, the main reason it happens is because we use the $PAGE->url as a reference to get the clean version, this is set by the developer of the page not by what was provided in the address bar.

While we could use the current requested address instead to get the 'clean' version of the URL, this still means that the developer of the page forgot to add the proper parameters to the $PAGE->set_url() call.

Do you have an example of a page that really uses the extra parameters but are missing?

roperto commented 6 years ago

This one has been fixed in development branch.