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

Expose url_path_for on HTTPConnection #2872

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

judahrand
Copy link

@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
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.

1 participant