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

Models need a url_for() #9

Closed
jace opened this issue Jul 19, 2012 · 6 comments
Closed

Models need a url_for() #9

jace opened this issue Jul 19, 2012 · 6 comments
Assignees

Comments

@jace
Copy link
Member

jace commented Jul 19, 2012

Flask's url_for is view-centric. Given a view name and parameters, it generates a URL. We often have to work the reverse: given a model instance, create a URL to its view. All BaseMixin models should have a url_for(view="view") method that implementations can override to return a URL.

To be decided: If a view (default value view, implying the primary public view) is not known by the model, should the method:

  1. Return None, implying no known view? or
  2. Raise an exception, making it easier to catch gaps in implementation?
@ghost ghost assigned jace Jul 19, 2012
@jace
Copy link
Member Author

jace commented Jul 19, 2012

Given the experience with SQLAlchemy's .first() vs .one(), where the former is almost always used because it fits the mental model better, I think None is the correct response.

@jace
Copy link
Member Author

jace commented Jul 19, 2012

Should the parameter to url_for be called view or action? The latter implies a URL to a view that performs a certain action. The dual use of view to mean both the URL endpoint and a default action of viewing (ie, read-only access) is confusing. Naming the parameter action helps alleviate the confusion.

@jace
Copy link
Member Author

jace commented Jul 19, 2012

One downside to having url_for in the model: it can't be used to retrieve the URL for a new action because there is no instance on which to call url_for. It can't be a classmethod either because the endpoint may need a container, which is not known unless explicitly passed.

This suggests that the new action must be in the scope of the container.

@jace jace closed this as completed in b83e03c Jul 19, 2012
@jace
Copy link
Member Author

jace commented Nov 21, 2017

This was supplemented with a declarative approach in #77 (implemented in #79).

@jace
Copy link
Member Author

jace commented Mar 20, 2018

We've since switched over from .first() to .one() (more often .one_or_none()) because it helped catch programmer errors with the lack of appropriate unique constraints.

Similarly, url_for() should raise BuildError instead of returning None when an unknown view is requested for.

@jace
Copy link
Member Author

jace commented Nov 16, 2018

@iambibhas Could you work on a PR that replaces return with raise BuildError here? It'll break some templates in various apps, but we don't know which, so once we deploy this we'll have to be alert and fix those apps as error reports come in.

if action not in self.url_for_endpoints:
# FIXME: Legacy behaviour, fails silently, but shouldn't. url_for itself raises a BuildError
return

jace added a commit that referenced this issue Nov 22, 2018
Also deprecate silent failure during a build error.
@jace jace closed this as completed Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants