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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #34028 -- Allow usage of script prefix in middleware init #16714

Closed
wants to merge 1 commit into from

Conversation

sarahboyce
Copy link
Contributor

@sarahboyce sarahboyce commented Apr 1, 2023

Ticket: https://code.djangoproject.com/ticket/34028

An issue people seem to be having is that Whitenoise wants to access the script prefix in the middleware initialisation (see here). This is giving an unexpected result.

I believe this kind of relates to this ticket: https://code.djangoproject.com/ticket/16734 and this PR: #5470

I'm not sure why this set_prefix=False option was created, @claudep I know this is quite a long time ago, but maybe you can add some context?

If we agree with this change, it means all internal usages of set_prefix are removed which probably means it makes sense to depreciate it 馃 I'm not sure, I've never depreciated anything

Note: asgi is also doing the sameset_prefix=False - I can remove from there also just not comfortable writing anything async so might need help with the test case

@@ -9,5 +9,5 @@ def get_wsgi_application():
Avoids making django.core.handlers.WSGIHandler a public API, in case the
internal WSGI implementation changes or moves in the future.
"""
django.setup(set_prefix=False)
django.setup()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have a request here, so script_prefix would be wrong in most of cases. ticket-16734 was about setting script prefix when we are outside of the request-response cycle e.g. in management command. As far as I'm aware, this cannot be done properly because script_prefix can change even between requests and should only be used dynamically. Maybe we should "wontfix" this ticket 馃 (\cc @apollo13).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we end up going for a won't fix, I can try and raise a pr to whitenoise to help resolve it on their side (I think they just need to move the logic from the middleware init)
We could also add a note in the get_script_prefix() docs warning against using it outside the request response cycle - or maybe the ticket itself is enough documentation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way I see it we should do the following: There is a real need to have some kind of URL outside the request cycle (sending emails which need to create a link etc). For constructing a correct MEDIA_URL / STATIC_URL we wanted to use the script prefix to allow for more dynamic changes (ie allow a user to move to a subpath without having to change multiple variables). Since this is all not workable I am really starting to lean towards introducing a setting (yes!) called BASE_URL that can be set to https://mysite.com/subpath. This would allow for a) generating URLs outside a request context and b) allow us to mostly ignore script prefix. We could also use a relative STATIC_URL and append it to BASE_URL. This way there would be just one setting that requires changing and many projects hopefully would make that configurable via env vars etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 馃憤
Would that suggestion need discussing in a wider group or do you think we can reuse the existing ticket or would it be cleaner to open a new one with the suggested feature? 馃

We could also add a note in the get_script_prefix() docs warning against using it outside the request response cycle

I will close this PR once I've sorted my todo list 馃憤 do we think this would add value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess a new ticket that links to the existing ticket(s) would make more sense. We certainly need to explain what BASE_URL would fix and how the migration to it would look like.

We could also add a note in the get_script_prefix() docs warning against using it outside the request response cycle

I will close this PR once I've sorted my todo list +1 do we think this would add value?

Not to sure, get_script_prefix is used so deep inside in Django that it is not really in control of the enduser?

Copy link
Contributor Author

@sarahboyce sarahboyce Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to sure, get_script_prefix is used so deep inside in Django that it is not really in control of the enduser?

It's a documented utility function which people could use and I guess the reason the ticket was raised initially was because of how an external package (whitenoise) was using it. Still think documentation is a balance thing and it currently reads well so I will leave it alone.

I raised a ticket https://code.djangoproject.com/ticket/34461
@apollo13 please feel free to update the title and description as you see fit 馃憤

Also @felixxm I'll let you decide if to accept the new ticket and close the older one 馃憤

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhm fair, I was only thinking of Django's use of the script prefix which can already cause problems in other areas (static url comes to mind). So yes, a docs amendment can probably indeed help :)

Thanks for the ticket!

@sarahboyce
Copy link
Contributor Author

As discussed, not the right approach 馃憤

@sarahboyce sarahboyce closed this Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants