Opened 8 years ago

Closed 8 years ago

#26086 closed Cleanup/optimization (wontfix)

Don't allow admin.site.register([]) or admin.site.unregister([])

Reported by: anabelensc Owned by: anabelensc
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: anabelensc Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Hello,

I think if you do that :
admin.site.register([]) or admin.site.unregister([]) the function register and unregister should complain.
I attach a pacth to try to fix this.

Thanks.

Attachments (1)

model_empty_list_registration_unregistration.diff (2.5 KB ) - added by anabelensc 8 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 by anabelensc, 8 years ago

Version: 1.9master

comment:2 by Aymeric Augustin, 8 years ago

I'm not convinced that a good idea.

Some users may be passing a possibly-empty-list to admin.site.register in order to avoid having to special-case the empty list.

comment:3 by Tim Graham, 8 years ago

Component: Uncategorizedcontrib.admin
Has patch: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Maybe we can remove the check added in #25688 too. Are you able to submit your patch as a pull request?

comment:4 by anabelensc, 8 years ago

Yes, I can
thank you

comment:5 by Tim Graham, 8 years ago

Aymeric, could you give an example of such a pattern? I can't come up with a situation where an empty list would be inadvertently passed either, so I don't care much either way.

in reply to:  5 comment:6 by Simon Charette, 8 years ago

Replying to timgraham:

Aymeric, could you give an example of such a pattern? I can't come up with a situation where an empty list would be inadvertently passed either, so I don't care much either way.

If the list consist of models that can be present or not in a third-party application setting with optional dependencies.

models = []
for app_label in ['optional1', 'optional2']:
    try:
        app = apps.get_app_config(app_label)
    except LookupError:
        continue
    models.extend(app.get_models())

I've not used apps this way personally but I've seen a couple of third party applications in the wild that rely on ImportError to register or not optional dependency models to the admin.

Even if this pattern is not common I agree with Aymeric that it's not a good idea. Plus this is backward incompatible and would require a deprecation which is a lot of trouble for a check that could raise false positives.

comment:7 by Tim Graham, 8 years ago

Resolution: wontfix
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top