Skip to content

Expose url_path_for on HTTPConnection#2872

Closed
judahrand wants to merge 6 commits intoKludex:masterfrom
judahrand:url_path_for
Closed

Expose url_path_for on HTTPConnection#2872
judahrand wants to merge 6 commits intoKludex:masterfrom
judahrand:url_path_for

Conversation

@judahrand
Copy link
Copy Markdown

@judahrand judahrand commented Feb 17, 2025

Summary

Based on the conclusion of #604 which can be found here the consensus seems to be that trying to munge host headers when located behind a reverse proxy isn't the best approach and that instead absolute paths should be used in redirects. The same, I imagine, should apply in tempates.

This is currently possible but would be made easier/neater if the url_path_for method which exists on Starlette was exposed on the HTTPConnection and therefore the Request. Additionally, adding a url_path_for jinja function would allow for this to be achieved in templates much easier.

I leave this PR here for review with the knowledge that if this approach is approved of docs will also need to be added.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@judahrand judahrand force-pushed the url_path_for branch 2 times, most recently from 68796fd to 6042901 Compare February 17, 2025 11:15
@alex-oleshkevich
Copy link
Copy Markdown
Contributor

self.url_for returns and instance of URL that you can use in routes/templates to get a path: self.url_for(name).path (you have it already done in the PR).
I don't think we need another method.

The difference between Starlette.url_path_for and HTTPConnection.url_for is that Starlette app can be used in non-HTTP contexts (like CLI or background tasks) and therefore has no Host concept. In all other cases I advise using url_for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants