Opened 6 months ago

Closed 5 months ago

#36268 closed Cleanup/optimization (fixed)

The querystring template tag should render an empty query string result as "?" instead of ""

Reported by: Sarah Boyce Owned by: Natalia Bidart
Component: Template system Version: dev
Severity: Normal Keywords:
Cc: David Feeley, Tom Carrick, Natalia Bidart Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Test to demonstrate the issue:

  • tests/template_tests/syntax_tests/test_querystring.py

    a b class QueryStringTagTests(SimpleTestCase):  
    5656                self.assertRenderEqual(
    5757                    "test_querystring_empty_params", context, expected=""
    5858                )
     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                )
    5966
    6067    @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.

Change History (5)

comment:1 by Natalia Bidart, 6 months ago

Owner: set to Natalia Bidart
Status: newassigned
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: UnreviewedAccepted
Version: 5.1dev

Thank you Sarah for the report. As we discussed in the context of #35529, I agree.

comment:2 by Natalia Bidart, 6 months ago

Has patch: set

comment:3 by David Feeley, 6 months 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

comment:4 by Sarah Boyce, 5 months ago

Triage Stage: AcceptedReady for checkin

comment:5 by nessita <124304+nessita@…>, 5 months ago

Resolution: fixed
Status: assignedclosed

In 0b4f2d8d:

Fixed #36268 -- Added leading ? in every querystring template tag result.

Thanks Sarah Boyce for the report.

Note: See TracTickets for help on using tickets.
Back to Top