Opened 3 years ago

Last modified 2 years ago

#24313 new Cleanup/optimization

Deprecate the class_prepared signal

Reported by: Aymeric Augustin Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Since 1.7, whatever can be done in a class_prepared listener is probably better done by inspecting the app registry in AppConfig.ready().

As the documentation notes, reacting to class_prepared is impractical because the app-loading and meta-refactor APIs don't work yet.

Even if we keep the signal — I once tried to refactor this area and it went downhill quickly — I think we should remove it from the public API.

Change History (8)

comment:1 Changed 3 years ago by Carl Meyer

Triage Stage: UnreviewedAccepted

I agree. ISTM that since app-loading, Django shouldn't even need class_prepared internally anymore - but I realize there are dragons in that code.

comment:2 Changed 3 years ago by Carl Meyer

For reference, the only place Django internally uses class_prepared is to call do_pending_lookups to handle unresolved relations to the just-loaded model. It certainly seems that could/should be handled by the app registry instead...

comment:3 Changed 3 years ago by Aymeric Augustin

Follow the link in the description for the story of the last time you sent me down this rabbit hole :-)

comment:4 in reply to:  2 Changed 3 years ago by Alex Hill

Replying to carljm:

For reference, the only place Django internally uses class_prepared is to call do_pending_lookups to handle unresolved relations to the just-loaded model. It certainly seems that could/should be handled by the app registry instead...

Hi Carl, I've opened ticket #24215 and written a patch to do just this - feedback welcome.

comment:5 Changed 3 years ago by Tim Graham

There's also a usage in ModelSignal. It's not immediately obvious to me how/if this can be refactored to use the app registry instead.

comment:6 Changed 2 years ago by Tim Graham

The class_prepared usage mentioned in my previous comment is removed as part of #26421 so it looks like this should be doable after that's merged.

comment:7 Changed 2 years ago by Alex Hill

Yep class_prepared is no longer used anywhere in Django other than tests AFAIK. Unless someone can outline a use that can't be implemented with AppConfig.ready(), I agree with deprecating it because

  • it's unlike other signals in that the trigger is generally expected to happen only once per sender
  • it's hard to make sure it's registered in the right place

Mezzanine uses class_prepared to add fields to models at boot time, which sounds nasty but works surprisingly well. Until Django has a sanctioned method of swapping models besides auth.User, we'll continue to support this. I'll try to refactor it to use AppConfig.ready() and see how that goes.

comment:8 Changed 2 years ago by Alex Hill

Alright, there is at least one gap in functionality: using class_prepared, you can fiddle with models in ways that propagate to their subclasses. In AppConfig.ready(), since all the ModelBase inheritance magic has already happened, it's too late to do anything like that.

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