Opened 3 years ago

Closed 2 years ago

Last modified 14 months ago

#31180 closed New feature (fixed)

Deprecate default_app_config.

Reported by: Aymeric Augustin Owned by: Aymeric Augustin
Component: Core (Other) Version: dev
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 (11)

comment:1 Changed 3 years ago by Mariusz Felisiak

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

comment:2 Changed 3 years ago by Aymeric Augustin

Has patch: set
Last edited 3 years ago by Mariusz Felisiak (previous) (diff)

comment:3 Changed 3 years ago by Sanskar Jaiswal

Owner: changed from nobody to Sanskar Jaiswal
Status: newassigned

comment:4 Changed 3 years ago by Mariusz Felisiak

Owner: changed from Sanskar Jaiswal to Aymeric Augustin

Sanskar, this ticket has a patch already.

comment:5 Changed 2 years 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 2 years 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 2 years 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 2 years ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:9 Changed 2 years ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In 3f2821af:

Fixed #31180 -- Configured applications automatically.

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

In 287e36b:

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

comment:11 Changed 14 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 75d6c4ae:

Refs #31180 -- Removed default_app_config application configuration variable per deprecation timeline.

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