Avoid using context.request directly in querystring template tag

[Jake Howard]

The RequestContext subclass sets context.request to the current request object. This attribute isn't available when Context is used, or when something context-looking is used instead. For example, the new querystring tag accesses context.request, making it incompatible in some cases.

I'd suggest that uses of context.request directly be replaced with context["request"], and that RequestContext automatically add request to its own context if it's not there already. It might also be worth renaming the attribute to _request to hint it shouldn't be used externally.

This likely only occurs when template context is being incorrectly handled, but it's likely worth improving regardless.

[jaffar Khan]

Like to work on it if accepted.

[Vidhi Singh]

This seems like a reasonable cleanup to avoid relying on context.request, which isn’t always present.
I’m happy to work on a patch if maintainers agree with the proposed direction.

[Natalia Bidart]

Hey Jake, thank you for the ticket. I think I have suffered from this issue when using querystring in a base template that is later used for a 500.html template, but I would like to understand your motivation/use case (or failing test case?) before accepting the ticket. My main concern is the backward incompatibility (and I have also removed the easy picking flag because of that).

[Jake Howard]

My main motivation is exactly that - that there are situations where the context in a template is a Context instance rather than RequestContext (unintentionally or otherwise). I think the primary time this comes up is when a template is rendered manually, rather than using Django's helpers. ​Wagtail has hit this issue in its StreamField, which renders templates directly, rather than as part of the final request rendering (ie using Context rather than the special RequestContext).

Having querystring (and other future tags) depend on having a request in the context makes sense, but assuming it is a RequestContext instance feels unnecessarily specific, especially since RequestContext is undocumented.

Backwards compatibility wise, since RequestContext isn't a public API, tweaking the RequestContext.request behaviour shouldn't be especially breaking. Similarly, users of querystring won't notice a difference, and if anything will see strange incompatibilities resolved.

[Natalia Bidart]

Thank you Jake, that makes sense!

[Yogya Chugh]

Hi ! I was about to assign it to myself when i noticed a PR from 3 days ago. Just curious if it's been worked on cause of the linter issues, or can i assign ?

[Jacob Walls]

Thanks for your interest, but three days isn't long enough to signal inactivity.

[Jacob Walls]

Ah, but I just looked and closed that PR, as it wasn't very close at all. Feel free to assign yourself after all!

[Yogya Chugh]

Sure, will get on it today !

[Yogya Chugh]

​PR

[Yogya Chugh]

I have replaced the request attribute with context["request"] in querystring and added 3 testcases and updated 1 for checking compatibility with Context object rather than being specific to RequestContext.
But,
As seen in the PR too, there is 1 testcase failing ( see below ) leading to a lot of failed checks. It's main cause is the "request" key added to context.

The test case fails on last line self.assertContains(response, '<a href="%s">Users</a>' % url) as it returns with an aria-current attribute due to the request.

So should I go forward with setting context["_request"] instead as it passes all tests but i feel it's temporary ?
( About the linter issues, fixed but will be added in next commit )

@override_settings(
        TEMPLATES=[
            {
                "BACKEND": "django.template.backends.django.DjangoTemplates",
                "DIRS": [],
                "APP_DIRS": True,
                "OPTIONS": {
                    "context_processors": [
                        "django.contrib.auth.context_processors.auth",
                        "django.contrib.messages.context_processors.messages",
                    ],
                },
            }
        ]
    )
    def test_sidebar_aria_current_page_missing_without_request_context_processor(self):
        url = reverse("test_with_sidebar:auth_user_changelist")
        response = self.client.get(url)
        self.assertContains(
            response, '<nav class="sticky" id="nav-sidebar" aria-label="Sidebar">'
        )
        # Does not include aria-current attribute.
        self.assertContains(response, '<a href="%s">Users</a>' % url)

[Jake Howard]

The _request reference is about changing the RequestContext class, rather than changing anything in the context directly. This should just be a case of renaming RequestContext.request to RequestContext._request, and updating any references.

[Yogya Chugh]

Replying to Jake Howard:
Yes, renaming request to _request attribute in RequestContext and updating other references makes sense and it's the next step but my concern is querystring.
Our goal ( in querystring ) is to replace usage like context.request so as not to assume it's a RequestContext. And as u mentioned in description,

I'd suggest that uses of context.request directly be replaced with context["request"], and that RequestContext automatically add request to its own context if it's not there already.

The testcase fail I talked in previous comment is due to RequestContext adding request to it's own context self.setdefault("request", request).
Going from request to _request attribute is a name change in multiple places and is perfectly fine but request being present in context triggers test_sidebar_aria_current_page_missing_without_request_context_processor because it doesn't expect request being in context when context processor is disabled.

So renaming request to _request in context was just a temporary fix to this, nothing else.
What are your thoughts ?

[Jake Howard]

RequestContext should by definition always have a request. The difference is that now it always adds the request into its context, which negates the need for the request context processor entirely. Context processors are ​run by RequestContext, which means if the context processors run, there will be a request in context.

I think the request context processor should be deprecated (with a warning, removed in 7.0), and the ​check implemented as part of #31575 can be removed. I can't think of a case where you'd explicitly not want request in the context, so deprecating it shouldn't be high impact. With that, the test_sidebar_aria_current_page_missing_without_request_context_processor test is no longer necessary.

Given the scope has changed a fair bit, this probably warrants an additional triage before continuing. Natalia, does the new deprecation sound sensible? The deprecation could be its own ticket, however the test would still need to be removed.

[Yogya Chugh]

Natalia, any updates on how you’d like to proceed here, given Jake’s suggestion?