Opened 7 months ago

Closed 3 weeks ago

Last modified 2 days ago

#31180 closed New feature (fixed)

Deprecate default_app_config.

Reported by: Aymeric Augustin Owned by: Aymeric Augustin
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: 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

Many pluggable applications are configured with a dotted path to the app (e.g. "django.contrib.admin") in INSTALLED_APPS.

The application then relies on default_app_config for locating its AppConfig class (e.g. "django.contrib.admin.apps.AdminConfig").

default_app_config was intended to allow author of pluggable applications to take advantage of AppConfig features without require their users to change INSTALLED_APPS, in order to increase adoption.

Unfortunately, that reduced the incentive for directly putting paths to AppConfig in INSTALLED_APPS. Even though that was the official recommendation, it didn't gain traction.

If I had better anticipated this result, I wouldn't have introduced default_app_config in the app-loading refactor in Django 1.7.

We've spent about five years failing at getting people to adopt explicit configuration. It's time to admit failure. Practicality beats purity.

With the benefit of hindsight, I'm proposing to load AppConfig classes from a conventional location, namely the apps submodule inside an application.

This is inconsistent with Django's design philosophy, as Django favors configuration over convention, but not much worse than the default_app_config convention.

There's a small risk of backwards-incompatibility. Django could import an apps.py submodule designed for another purpose and that could have side effects. I think that risk is low.

Change History (10)

comment:1 Changed 7 months ago by felixxm

Component: Core (Serialization)Core (Other)
Summary: Deprecate default_app_configDeprecate default_app_config.
Triage Stage: UnreviewedAccepted

comment:2 Changed 7 months ago by Aymeric Augustin

Has patch: set
Last edited 7 months ago by felixxm (previous) (diff)

comment:3 Changed 7 months ago by Sanskar Jaiswal

Owner: changed from nobody to Sanskar Jaiswal
Status: newassigned

comment:4 Changed 7 months ago by felixxm

Owner: changed from Sanskar Jaiswal to Aymeric Augustin

Sanskar, this ticket has a patch already.

comment:5 Changed 4 weeks ago by Carlton Gibson

Triage Stage: AcceptedReady for checkin

Needs a final squash, but looks good.

Pushed some edits so will just give Aymeric a chance to comment again.

comment:6 Changed 4 weeks ago by Carlton Gibson

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

This isn't quite ready.

There's a test failure from the new check, for a test added in dd1ca50b096bf0351819aabc862e91a9797ddaca (after the PR here was first created):

======================================================================
ERROR: test_translation_loading (i18n.tests.TranslationLoadingTests)
"loading_app" does not have translations for all languages provided by
----------------------------------------------------------------------
Traceback (most recent call last):
...
  File "django/django/apps/registry.py", line 354, in set_installed_apps
    self.populate(installed)
  File "django/django/apps/registry.py", line 91, in populate
    app_config = AppConfig.create(entry)
  File "django/django/apps/config.py", line 235, in create
    app_name, app_config_class.__module__, app_config_class.__name__,
django.core.exceptions.ImproperlyConfigured: Cannot import 'loading_app'. Check that 'i18n.loading_app.apps.LoadingAppConfig.name' is correct.

I suspect that's just an artifact of the test case.

Plus, there are a couple of new review comments to double-check.

comment:7 Changed 4 weeks ago by Aymeric Augustin

Patch needs improvement: unset

I believe all feedback was addressed and the PR just needs squashing and merging.

comment:8 Changed 3 weeks ago by felixxm

Triage Stage: AcceptedReady for checkin

comment:9 Changed 3 weeks ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In 3f2821af:

Fixed #31180 -- Configured applications automatically.

comment:10 Changed 2 days ago by GitHub <noreply@…>

In 287e36b:

Refs #31180 -- Fixed unreachable assertions in apps tests.

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