`relative_url_for` trips on anchors #30

Closed
homeworkprod opened this Issue May 26, 2013 · 5 comments

Projects

None yet

2 participants

@homeworkprod
Contributor

The relative_url_for method introduced in version 0.10 uses the naïve approach of appending index.html if an URL ends with a slash.

This, however, fails for URLs with an anchor (e. g. http://wiki.example.com/pages/Bacon#Positive-Effects-on-the-Human-Mind or /items/123/#some-superb-item).

Flask introduced the _anchor keyword argument to url_for (http://flask.pocoo.org/docs/api/#flask.url_for) in version 0.9.

@homeworkprod
Contributor

Here is my attempt at fixing the issue: homeworkprod/Frozen-Flask@7fd0220

The patch seems to solve the issues I encountered. However, five tests still fail. Also, as URLs are aggregated in a set, further changes and maybe design decisions might be necessary regarding the uniqueness of an URL (so this might or might not include the anchor).

Special attention is required for the two cases I gave examples for above (the key part here is the slash [not] placed right before the hash mark); the tests probably need to be extended to cover both cases (note that one, as opposed to the other one, should result in a directory being created).

Apart from that, I wonder what will happen to query strings …

@SimonSapin
Collaborator

I think that .partition('#') is the right way to parse the fragment/anchor, but you could just remove it. URLs that only differ by their fragments should be considered the same for this purpose; the fragment is removed at some point before your app does URL matching.

Could you make a pull request? (Tiny nitpick, I’d rather do a, _, b = t rather than a, b = t[::2].)

As to query strings, you’re right to wonder. URLs that differ by their query strings are not the same (you app could respond with different content based on the query string.) The problem is that they would map to the same "frozen" filename, which means that the one generated last would overwrite the file, and the order is not defined. I suppose the best we can do is write "Don’t do that, the results are undefined." in the docs.

@homeworkprod
Contributor

Here is the PR, though incomplete: #32
Feel free to work on it.

I agree that URLs are basically the same if they only differ in anchors and query arguments.

FYI, my (maybe kinda special) case is that I want slugs in my URLs, but I want to be able to change them afterwards without breaking (as in 404) "perma"links. Because this doesn't apply to /articles/123-groundbreaking-stuff I came up with /articles/123/#groundbreaking-stuff, i.e. the slug is set as an anchor and changing it therefor still allows to access the resource (which itself doesn't usually contain an element with the ID that is represented by the anchor).
The reason the trailing slash is in there is that images and downloads are in the same directory as the actual article (in this example), so the index.html generated by Frozen-Flask should be as well.

As a result, I'm not sure if dropping the anchor altogether is what I want – at least not in the final generated build that does not use relative URLs; just for local development without a web server and using relative URLs, dropping the anchor might be ok, though.

@SimonSapin
Collaborator

URLs are "the same" if they differ only by fragment, but not if they differ by query string!

Also I only meant to suggest to drop fragments for Frozen-Flask's purpose of finding what to build, not for the links generated in HTML.

@SimonSapin
Collaborator

Ok, I didn’t pay enough attention and didn’t notice this was about relative_url_for. So parts of what I wrote above don’t make sense, sorry about that.

@SimonSapin SimonSapin closed this in f6ac102 Jun 4, 2013
@jperkin jperkin pushed a commit to joyent/pkgsrc that referenced this issue Dec 9, 2013
kleink Update www/py-flask-frozen to 0.11.
Version 0.11
~~~~~~~~~~~~

Released on 2013-06-13.

* Add Python 3.3 support (requires Flask >= 0.10 and Werkzeug >= 0.9)
* Drop Python 2.5 support
* Fix `#30 <Frozen-Flask/Frozen-Flask#30:
  :func:`relative_url_for` with a query string or URL fragment.
8664427
@jperkin jperkin pushed a commit to joyent/pkgsrc that referenced this issue Jan 21, 2014
kleink Update www/py-flask-frozen to 0.11.
Version 0.11
~~~~~~~~~~~~

Released on 2013-06-13.

* Add Python 3.3 support (requires Flask >= 0.10 and Werkzeug >= 0.9)
* Drop Python 2.5 support
* Fix `#30 <Frozen-Flask/Frozen-Flask#30:
  :func:`relative_url_for` with a query string or URL fragment.
a1c2575
@jperkin jperkin pushed a commit to joyent/pkgsrc that referenced this issue Mar 14, 2014
kleink Update www/py-flask-frozen to 0.11.
Version 0.11
~~~~~~~~~~~~

Released on 2013-06-13.

* Add Python 3.3 support (requires Flask >= 0.10 and Werkzeug >= 0.9)
* Drop Python 2.5 support
* Fix `#30 <Frozen-Flask/Frozen-Flask#30:
  :func:`relative_url_for` with a query string or URL fragment.
ed896dc
@jsonn jsonn pushed a commit to jsonn/pkgsrc that referenced this issue Oct 11, 2014
kleink Update www/py-flask-frozen to 0.11.
Version 0.11
~~~~~~~~~~~~

Released on 2013-06-13.

* Add Python 3.3 support (requires Flask >= 0.10 and Werkzeug >= 0.9)
* Drop Python 2.5 support
* Fix `#30 <Frozen-Flask/Frozen-Flask#30:
  :func:`relative_url_for` with a query string or URL fragment.
ed26bd2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment