Code

Opened 4 months ago

Closed 3 months ago

#21712 closed Cleanup/optimization (fixed)

Move admin.autodiscover() to AppConfig.setup()

Reported by: aaugustin Owned by: aaugustin
Component: contrib.admin Version: master
Severity: Normal Keywords: app-loading
Cc: Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by aaugustin)

It doesn't belong to urls.py!

This probably requires shipping an apps.py with an AppConfig in the default project template and adding lots of documentation, including in the tutorial.

Attachments (0)

Change History (13)

comment:1 Changed 4 months ago by aaugustin

  • Description modified (diff)

In fact, we don't have a good API for project-level startup code. Many projects have a "project app" for projectwide templates, staticfiles, etc. but the tutorial doesn't. This requires a bit more thought.

comment:2 Changed 4 months ago by aaugustin

It may be more appropriate to put it in the admin AppConfig (once there's one, #21675) and to document how do disable it (via subclassing).

comment:3 Changed 4 months ago by aaugustin

Summarizing a discussion with Marc on IRC:

  • https://gist.github.com/mjtamlyn/8209667
  • explain in the release notes to use 'django.contrib.admin.app.AdminConfig' instead of 'django.contrib.admin' in INSTALLED_APPS and remove admin.autodiscover() from urls.py.

comment:4 Changed 4 months ago by mjtamlyn

PR submitted. There are at least three things to consider which are in the PR - including naming of the file, default project and testing.

https://github.com/django/django/pull/2136

comment:5 Changed 4 months ago by bpeschier

I would change the default project to use the AdminConfig. My guess is most people use the autodiscover method. People implementing their own AdminSite can read the documentation about it.

comment:6 Changed 3 months ago by claudep

+1 to change the default project.
I'd keep the app.py name. apps.py would suggest several applications.
I'd also add verbose_name = _("Django administration").

comment:7 Changed 3 months ago by mjtamlyn

The last point about the verbose_name is tracked in #21675 as it covers all contrib apps.

comment:8 Changed 3 months ago by aaugustin

Yes, we must change the default project template. We should also copy-paste the usual warnings in the tutorial for people who don't check properly what version they're using.

We've decided to always use apps.py. Let's stick with that.

comment:9 Changed 3 months ago by aaugustin

  • Owner changed from nobody to aaugustin
  • Status changed from new to assigned

comment:10 Changed 3 months ago by aaugustin

It's impossible to make this change in the test suite because it relies on the admin autodiscovery not being run.

The bug8245 test app contains an invalid admin module that breaks autodiscovery.

comment:11 Changed 3 months ago by aaugustin

  • Triage Stage changed from Accepted to Ready for checkin

New PR: https://github.com/django/django/pull/2182

I think it's good to go but I would appreciate a proof-read. It's mostly a documentation patch.

comment:12 Changed 3 months ago by claudep

I've only read the patch, but it looks good to me (except reported typos).

comment:13 Changed 3 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 10e0cfc0e4e77b85e8261c908baf1c4814ded3f8:

Fixed #21712 -- Moved autodiscover() to AdminConfig.ready().

Thanks Marc Tamlyn for the initial version of the patch.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.