Opened 35 hours ago
Last modified 31 hours ago
#36268 assigned Cleanup/optimization
The querystring template tag should render an empty query string result as "?" instead of ""
Description ¶
Test to demonstrate the issue:
-
TabularUnified tests/template_tests/syntax_tests/test_querystring.py
a b class QueryStringTagTests(SimpleTestCase): 56 56 self.assertRenderEqual( 57 57 "test_querystring_empty_params", context, expected="" 58 58 ) 59 request = self.request_factory.get("/?a=1") 60 for param in cases: 61 with self.subTest(param=param, url="/?a=1"): 62 context = RequestContext(request, {"qd": param}) 63 self.assertRenderEqual( 64 "test_querystring_empty_params", context, expected="?" 65 ) 59 66 60 67 @setup({"querystring_replace": "{% querystring a=1 %}"})
In short, returning an empty string for querydict means that the page would not reload if this was used in a link, see #36182.
Therefore, it only makes sense to return an empty string when the query string that would be evaluated is equivalent to the current query string.
We are not supporting this in most cases e.g. if you are on /?color=green
and in the template there is {% querystring color=green %}
this evaluates to "?color=green"
(not ""
).
In the niche case that you are on /?color=green
and pass in an empty query dict rather than the default request.context.GET
, and render {% querystring query_dict %}
this returns ""
and so the page would not reload and you would stay on /?color=green
even though you probably expected this to blitz the query string and take you to /
(which would be the case if "?"
was returned).
I think this is niche because there is no reason for you not to hard code the url in these cases as you don't care about the current query string
For simplicity, I think the proceeding "?" should always be returned.
I think the "bug" I described is unlikely to happen in the wild, I am classing this as a cleanup. Other thoughts are welcome.
According to the ticket's flags, the next step(s) to move this issue forward are:
- For anyone except the patch author to review the patch using the patch review checklist and either mark the ticket as "Ready for checkin" if everything looks good, or leave comments for improvement and mark the ticket as "Patch needs improvement".
Change History (3)
comment:1 by , 35 hours ago
Owner: | set to |
---|---|
Status: | new → assigned |
Summary: | Always render the empty querystring templatetag should render empty querystring as "?" not "" → The querystring template tag should render an empty query string result as "?" instead of "" |
Triage Stage: | Unreviewed → Accepted |
Version: | 5.1 → dev |
comment:3 by , 31 hours ago
Hi, for whatever its worth, I like this change, conceptually an empty query string to me should be '?', and not , given the way empty hrefs mean to reload the current page.
thanks!
Dave
Thank you Sarah for the report. As we discussed in the context of #35529, I agree.