adding _scheme parameter to url_for

[maxcountryman]

In order to better facilitate generation of URLs that make use of an HTTPS URL
scheme this patch adds a parameter with this specific purpose in mind. To
achieve this we explicitly pass in a param, _scheme='https', and then set the
url_scheme attribute of our MapAdapter instance appropriately.

Importantly, _external=True must be set in order for this to work properly.
As such, failure to do so results in a ValueError being raised.

[xsleonard]

I'm generating urls in some of my email message templates (confirmation, password reset), and I would like them to be secure urls, but the source from where they are generated is not necessarily https.

Current I am doing this to handle the problem,
https://gist.github.com/4561140

But it would be nice to not need this workaround

[untitaker]

I think the new keyword argument of url_for is a good idea, but I don't see the point of secure_url_for. Also I think the _secure arg should imply _external. Explicitly setting _external to False and _secure to True would result in a ValueError.

[maxcountryman]

I'm happy to omit secure_url_for if the _secure flag is deemed sufficient. It's trivial to enforce _external btw, which incidentally secure_url_for does do. However, a ValueError is seemingly the appropriate response where a user explicitly passes in _secure=True and _external=False: this is in fact an error and should raise an error. It would be sneaky and a little confusing to override an explicit pass in of _external=False imo.

[untitaker]

Could you add the handing for url_for(..., _secure=True, _external=False) please?

[maxcountryman]

@untitaker do you mean raise an explicit ValueError, e.g. ValueError('_external must be True where _secure is True')? Otherwise I do think raising a ValueError makes sense, but it might be best to do it explicitly in the scope of url_for.

[untitaker]

Raising a ValueError in url_for and implying _external when _secure is True in url_for seems best to me. As i said, adding a new function for this is not a good idea IMO.

[untitaker]

What does the overlord of elegant APIs @kennethreitz have to say about this?

[maxcountryman]

I don't know that it hurts to provide a helper function, after all, that's what helpers are there for. But as I said before, I'll defer to the maintainers. I'd just like to get this in as there seems to be a need for it. Out of curiosity, what relationship do you have to the Flask project, @untitaker?

[kennethreitz]

Why not just url_for(secure=True) ?

[untitaker]

About as much as you have @maxcountryman

[untitaker]

@kennethreitz The fear is that this could interfere with the user's keyword arguments for the view.

[maxcountryman]

@kennethreitz I think url_for(..., _secure=True) is more consistent with the API fwiw.

[kennethreitz]

that would be a terrible kwarg :)

[maxcountryman]

@kennethreitz so is _external=True, but that's what we have. :)

[maxcountryman]

I think at this point it's probably worse to be inconsistent with the existing API than to try to retroactively correct it. So for that reason I preferred _secure.

I still need feedback on whether or not to include secure_url_for as a helper from @kennethreitz or @mitsuhiko.

Finally I can include an explicit ValueError within the scope of url_for if it's not sufficient to leave this lower levels of the call stack.

How does that sound?

[untitaker]

Another aspect: I find "_secure" as an arg too ambiguous, since it also could mean that the url will be escaped to prevent XSS or something like that. "_https" seems like a better name (and also https_url_for)

[untitaker]

@maxcountryman +1

[kennethreitz]

(_scheme=)

[kennethreitz]

The more I think about this, the more I feel it is unnecessary, I suppose. If you want something to be SSL, why wouldn't everything already be SSL?

[maxcountryman]

@kennethreitz read @xsleonard's comment for real world use-case.

[kennethreitz]

hmm, I guess I like url_for(_scheme='https') then

[maxcountryman]

@kennethreitz how about a helper? Yea or nay? (I don't have a particularly strong opinion, but it does seem like a clear shortcut and it clearly exposes the functionality.)

[untitaker]

Agreed Kennneth, a scheme arg might be even better.

[maxcountryman]

I've force-pushed some updates to this guy. I think this should be more in line with your suggestions @kennethreitz.

[kennethreitz]

@mitsuhiko thoughts?

[kennethreitz]

Seems harmless enough.