#35316 closed Uncategorized (wontfix)
Support scalars as query parameters in admin changelist filters (for backward compatibility).
Reported by: | Manolis Stamatogiannakis | Owned by: | nobody |
---|---|---|---|
Component: | contrib.admin | Version: | 5.0 |
Severity: | Normal | Keywords: | |
Cc: | Sarah Boyce | Triage Stage: | Unreviewed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
Ticket #1873 added support for multi-valued query parameters in admin changelist filters. Because only list types are supported, older code that e.g. extends SimpleListFilter or BooleanFieldListFilter will break.
The latter (extending BooleanFieldListFilter) may even be completely unsupported. See notes in PR for #1873.
Overall, user code needs to introduce version conditionals if it is required to support both Django 4.x and Django 5. The reason is that Django 4.x only supports scalar types as query parameter values, while Django 5 only supports lists.
Since supporting both scalars and lists should be possible without adding complexity to the Django codebase, it would be good to continue supporting scalars, perhaps with a deprecation warning, if necessary.
Change History (4)
follow-up: 2 comment:1 by , 9 months ago
Cc: | added |
---|---|
Resolution: | → invalid |
Status: | new → closed |
comment:2 by , 9 months ago
Resolution: | invalid |
---|---|
Status: | closed → new |
Replying to Mariusz Felisiak:
build_q_object_from_lookup_parameters()
is a private API and all built-in filter useGET.lists()
so I don't see anything breaking in the #1873. If you extend built-in filters and really want to usebuild_q_object_from_lookup_parameters()
you should also useGET.lists()
.
I believe you may have misunderstood the case described. Of course build_q_object_from_lookup_parameters()
is not directly used, since I am referring to code that pre-dates Django 5, when the function was introduced. The problem is that this code now needs version conditionals in order to work with both Django 4.x and Django 5.x.
This isn't anything super fancy. Just a subclass of BooleanFieldListFilter that needs to have True as the default selection instead of All. It looks like this ATM:
class BooleanDefaultTrueFilter(BooleanFieldListFilter): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if self.lookup_kwarg2 not in self.used_parameters and self.lookup_kwarg not in self.used_parameters: self.`[self.lookup_kwarg] = prepare_lookup_value( self.lookup_kwarg, True if django.VERSION < (5,0) else (True,) ) self.lookup_val = '1'
Without the conditional, an exception will be raised in build_q_object_from_lookup_parameters()
, which is why the specific function was updated.
Of course one may still argue that used_parameters
is internal state. But overriding __init__()
like above is still the most elegant way I could find to achive the desired effect (see [1]). Is there a better/"sanctioned" way to achieve this effect? The documentation for BooleanFieldListFilter/FieldListFilter is pretty sparse.
Also, a quick GH search shows a number of repositories modifying used_parameters
[2]. So, although not thorougly documented, this is not something super-exotic for users customizing admin filters, and one would expect that there will be more codebases that will have to implement similar workarounds.
Overall, IMHO, the sparse documentation for BooleanFieldListFilter/FieldListFilter should not be a reason for breakage, when that is easily avoidable.
Along this line, I believe that updating build_q_object_from_lookup_parameters()
as proposed in PR-17990 will be a useful addition, as long as there are no side-effects that I missed and would outweigh the benefits. And as far as the test suite goes, it appears there aren't.
[1] https://forum.djangoproject.com/t/django-admin-filter-how-to-default-to-no-or-yes-rather-than-all/6684
[2] https://github.com/search?q=self.used_parameters%5B+lang%3APython+&type=code
follow-up: 4 comment:3 by , 9 months ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Are you maintaining a 3rd-party package? it's rather rare that you have to support multiple Django versions in a project. Moreover, prepare_lookup_value()
is another private API that you don't have to use. Why don't you set a value directly?
self.used_parameters[self.lookup_kwarg] = True
Please don't reopen closed ticket. You can follow the triaging guidelines with regards to wontfix tickets and take this to DevelopersMailingList, if you don't agree.
comment:4 by , 9 months ago
Replying to Mariusz Felisiak:
Why don't you set a value directly?
self.used_parameters[self.lookup_kwarg] = True
For the benefit of anyone else that may arrive here having a similar problem, this still needs a version-based conditional, because it still runs before build_q_object_from_lookup_parameters()
processes the values.
If prepare_lookup_value()
is ever made a sanctioned API, maybe it would be a more appropriate location to provide a fix for keeping existing Django 4 filter customization code working in Django 5.
EOT
build_q_object_from_lookup_parameters()
is a private API and all built-in filter useGET.lists()
so I don't see anything breaking in the #1873. If you extend built-in filters and really want to usebuild_q_object_from_lookup_parameters()
you should also useGET.lists()
.