Re: [Django] #36909: Avoid using context.request directly in querystring template tag

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: (none)
Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage: Accepted
requestcontext |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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 ?
--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:9>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: (none)
Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage: Accepted
requestcontext |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Yogya Chugh ):

* cc: Yogya Chugh (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:10>

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: (none)
Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage: Accepted
requestcontext |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Thanks for your interest, but three days isn't long enough to signal
inactivity.
--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:11>

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: (none)
Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage: Accepted
requestcontext |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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!
--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:12>

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------

Reporter: Jake Howard | Owner: Yogya
| Chugh

Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage: Accepted
requestcontext |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Yogya Chugh ):

* owner: (none) => Yogya Chugh

Comment:

Sure, will get on it today !
--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:13>

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: Yogya
| Chugh
Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage: Accepted
requestcontext |

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Yogya Chugh ):

* has_patch: 0 => 1

Comment:

[https://github.com/django/django/pull/20710 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:14>

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: Yogya
| Chugh
Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage: Accepted
requestcontext |
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jake Howard):

* needs_better_patch: 0 => 1
* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:15>

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: Yogya
| Chugh
Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage: Accepted
requestcontext |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:16>

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: Yogya
| Chugh
Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage: Accepted
requestcontext |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:17>

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: Yogya
| Chugh
Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage: Accepted
requestcontext |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Yogya Chugh ):

Replying to [comment:17 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 ?
--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:18>

[Django]

#36909: Avoid using context.request directly in querystring template tag
-------------------------------------+-------------------------------------
Reporter: Jake Howard | Owner: Yogya
| Chugh
Type: Bug | Status: assigned
Component: Template system | Version: 5.1
Severity: Normal | Resolution:
Keywords: querystring | Triage Stage:

requestcontext | Unreviewed

Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jake Howard):

* stage: Accepted => Unreviewed

Comment:

`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
[https://github.com/django/django/blob/fb3a11071aae27ef869d2b029289b9f59cc41128/django/template/context.py#L263
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
[https://github.com/django/django/commit/d522b51c401429c169d88742178a9b3777903d9e
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/36909#comment:19>