Opened 9 years ago

Closed 6 years ago

#544 closed defect (wontfix)

Provide .url_for_self for the Base model

Reported by: spaetz Owned by:
Priority: minor Milestone:
Component: programming Keywords:
Cc: Parent Tickets:

Description

# We have to pass in an "urlgen" object to MediaEntry().url_for_self() to actually get the url. Being attached to the request, this means we have to carry around the request object all over the place rather than having that encapsulated nicely. Compare to django's "get_absolute_url()" which does not need such complexities. (I also think we should implement a get_absolute_url template in the Base model class, to enforce uniformity by the way).

# in contrast to werkzeug tools, this always returns relative URLs, so the URL generation in the atom feed that requires absolute urls looks for example:

werkzeug.urls.Href(self.request.host_url (
    media_entry.url_for_self(self.request.urlgen)))

when what I want is:

media_entry.url_for_self(external=True)

(or even better a more consistent media_entry.get_absolute_url())
It should be possible to enforce absolute external urls easily without having to manually parse and fudge the request in every caller.

# Nitpick: MediaEntry.url_for_next and url_for_prev are defined in models.py, while url_for_self is defined in mixin.py. This should be made consistent.

Things would be easier if the url generation did not depend on the request-specific MapAdapter (request.urlgen), which it currently does. Not sure, how this could be improved.

To sum up:

  • At the very least, this bug should be about adding an external=True parameter that will return absolute, external URLs.
  • The url generation should be defined in the same places (mixin or models)
  • Optionally, we would implement a "get_absolute_url" in the Base model that all subclasses can override, so we can use the same consistent interface for all.This could be the same as "url_for_self" here for now.
  • Ideally, we don't have to pass in the request into the url_for_self function so we don't always have to carry that around and to simplify things. However, I don't see an easy way to fix that, so this could be a step 2 of this ticket.

Subtickets

Change History (6)

comment:1 Changed 9 years ago by spaetz

PS. One of the reasons I dislike the API is that it is not easy to understand what kind of thing "urlgen" is without having to dig deep into the internals and understanding werkzeug.If possible, I would like to avoid having to pass in "urlgen" as an argument in templates when it is usually used as a function to create the URLs in templates. This feels quite confusing to template writers (at least it did for me when I tried to extend templates).

comment:2 Changed 9 years ago by spaetz

Aye, Elrond told me about that "qualified=True" parameter which alleviates much of my concerns, at least I don't have to fudge with URLs anymore.

So, can we do an url_for_self without having to pass in "urlgen" as parameter? If we decide not, we should close this ticket. I'll see that I fix "MediaEntry().thumb_url to be a method that also allows to create qualified URLs.

comment:3 Changed 8 years ago by Elrond

Some notes:

  • To get url_for_self without param, we'd need to have a global urlgen or even better current request. We could have that via a thread local request thing. I am not entirely sure, whether this is a good idea. cwebber doesn't like it either: "it seems messy... I'm hesitant to add extra globals where we can avoid them, even though yeah we have them in a lot of places"
  • That said, I am not sure on how to improve the url_for_self API. Any suggestions? It feels okayish to me.
  • #686 is a bit related.

I think, if we don't see anything on this ticket in the next 8 weeks, we can close it.

comment:4 Changed 8 years ago by spaetz

Priority: majorminor
Summary: MediaEntry.url_for_self is horrible, horrible APIProvide .url_for_self for the Base model

OK, I am convinced by the technical difficulties (although this does not make the API look nice).

But it would still be great to provide an abstract "url_for_self" in the Base model, so that individual classes can override that as necessary. Makes sense?

comment:5 Changed 6 years ago by Elrond

I am not a python guru on that specific part:

  • Pro: The .url_for_self method on Base would have to raise NotImplemented anyway, as the Base has no url. So it would serve as a "this is the API you have to override, if you want to". So this is basicly a documentation helper.
  • Con: This is not C++, where the base class has to define all methods that can be called. An object is a writeable file, if it has a .write method. So any object with an .url_for_self is an url-for-self-object.

I tend to the "con" part. But I really don't know.

comment:6 Changed 6 years ago by spaetz

Resolution: wontfix
Status: newclosed

Ok, convinced.

Note: See TracTickets for help on using tickets.