-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
Fixed #36653 -- FORCE_SCRIPT_NAME is not respected for static URLs. #20305
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
|
It looks like the test leaked. This didn't happen on Linux. Let me fix that. |
This reverts commit 7dbbe65. Thanks to Brian Helba for the report and suggested solution, and Jacob Walls for the triage.
e5f1a32 to
3c6a0e6
Compare
📊 Coverage Report for Changed FilesNote: Missing lines are warnings only. Some lines may not be covered by SQLite tests as they are database-specific. For more information about code coverage on pull requests, see the contributing documentation. |
|
I think this fix just breaks stuff in the other direction. Now |
|
@apollo13 I believe this shouldn't be an issue because the request is handled in one thread in WSGI, or one task in ASGI. So when the request handler sets the script prefix, As for |
|
With the existing behavior the setting is cached once it is accessed from the main thread and then also available in background threads without a request context -- or am I missreading the code? Your changes would break that.
…On Mon, Nov 24, 2025, at 21:16, Xingzhe Li wrote:
*anusarati* left a comment (django/django#20305)
<#20305 (comment)>
@apollo13 <https://github.com/apollo13> I believe this shouldn't be an
issue because the request is handled in one thread in WSGI, or one task
in ASGI. So when the request handler sets the script prefix,
`STATIC_URL` will use that prefix throughout the response lifecycle,
which is the intended behavior of `FORCE_SCRIPT_NAME` and `SCRIPT_NAME`.
As for `BASE_URL`, I think we need to fix the existing behavior of
`FORCE_SCRIPT_NAME` to align with documentation before discussing
architectural changes.
—
Reply to this email directly, view it on GitHub
<#20305 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAT5C5KXGZC3SXD3UU2TRT36NRRPAVCNFSM6AAAAACM7FXUU2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTKNZSGU2TIOJVHA>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@apollo13 This fix does not break anything. Let me explain. The way it works before the fix, as you said, is that the main thread caches the In this fix, the "If This is because the correct value of the prefix is retrieved when attempting to access Regarding |
|
Yes, accessing I think we can accept a So how to fix the issue at hand? Since we shouldn't derive settings from anything WSGI environ related (and in hindsight we should never have merged the relative URL patch), one option would be to "force" users to use IMO, instead of trying to fix this issue like the current PR suggests we should get back to a state where we don't need to access the script prefix thread local at all -- if that means removing this broken feature, so be it. We don't have to cling on to something that was a very bad idea. |
I understand your hesitation regarding dynamic settings. Generally, I agree that settings should be immutable. However, Regarding the path forward:
I agree that in the long term (perhaps Django 6.0), a |
That proves nothing, the method was solely added to support the "dynamic nature" of
If I am reading the code correctly, this behavior exists since 2021. So people are relying on As said before, I am really worried about making dynamic settings a supported thing (which we kinda would by fixing that) instead of saying: "Oh we did wrong here, it doesn't work since four years anyways, so let us get rid of it" |
The "behavior" that has existed since 2021 is inconsistent, and people are most likely relying on being able to concatenate import io
from django.conf import settings
from django.core.wsgi import get_wsgi_application
from django.http import HttpResponse
from django.urls import path
if not settings.configured:
settings.configure(
ROOT_URLCONF=__name__,
INSTALLED_APPS=[],
STATIC_URL='static/',
FORCE_SCRIPT_NAME='/app/',
)
# View that returns the calculated STATIC_URL
urlpatterns = [
path('', lambda request: HttpResponse(settings.STATIC_URL)),
]
application = get_wsgi_application()
environ = {
'REQUEST_METHOD': 'GET',
'PATH_INFO': '/',
'wsgi.input': io.BytesIO(b""),
'SERVER_NAME': 'test',
'SERVER_PORT': '80',
}
response = application(environ, lambda s, h: None)
static_url = response.content.decode()
print(f"Output: {static_url}")
assert static_url == "/app/static/"This correctly prefixes the You may be wondering, what if people had code with a relative |
This reverts commit 7dbbe65.
Thanks to Brian Helba for the report and suggested solution, and Jacob Walls for the triage.
Trac ticket number
ticket-36653
Branch description
We no longer cache the prefixed STATIC_URL or MEDIA_URL because they depend on thread-local values.
Checklist
mainbranch.