Opened 14 months ago

Closed 12 months ago

Last modified 12 months ago

#22029 closed Cleanup/optimization (fixed)

documentation: new best place to put the signal handler registration

Reported by: un33k Owned by: nobody
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: vneekman@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The new best place for registering the signal handlers are in the ready() of the AppConfig.

This way, the signals properly emit with the server starts and also via the django-admin shell.

The documentation would benefit from an update indicating the new feature of Django 1.7.

The following section may require the update.
https://docs.djangoproject.com/en/dev/ref/signals/#django.db.models.signals.pre_migrate
https://docs.djangoproject.com/en/dev/ref/signals/#django.db.models.signals.post_migrate

This:
"Any handlers that listen to this signal need to be written in a particular place: a management module in one of your INSTALLED_APPS. If handlers are registered anywhere else they may not be loaded by migrate."
Could be changed to:
"Any handlers that listen to this signal need to be written in a particular place: the AppConfig.ready() within the apps module in one of your INSTALLED_APPS. If handlers are registered anywhere else they may not be loaded by migrate."

Attachments (1)

22029.diff (3.5 KB) - added by timo 12 months ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 14 months ago by un33k

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Also:

This:
"Sent by the migrate command before it starts to install an application."
Could be changed to:
"Sent by the migrate command before it starts to install an application that has a models module."

comment:2 Changed 14 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted

Changed 12 months ago by timo

comment:3 Changed 12 months ago by timo

  • Has patch set

I wonder if we need to mention this as something specific to these signals though? Can we assume all signals will now be registered in AppConfig.ready()?

comment:4 Changed 12 months ago by aaugustin

I didn't check the commit logs, but the wording sounds like this is a warning against registering signals in urls.py (for instance). I don't think these signals are a special case any more.

comment:5 Changed 12 months ago by un33k

The current docs says that you "need to" register these signals in the management module or else.

This is not the case anymore ... AppConfig.ready() is a better place, right Aymeric?

Also, circular dependencies are no longer the issue if these signals are registering in AppConfig.ready() and yes, no need to register in urls.py are more.

In short: "need to" is no longer true.

comment:6 Changed 12 months ago by Tim Graham <timograham@…>

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

In 5233b366935dcc81e47546921ab4eb5007c83719:

Fixed #22029 -- Removed obsolete advice on registering migrate/syncdb signal handlers.

All signals should now be registered in AppConfig.ready().

Thanks un33k for the report.

comment:7 Changed 12 months ago by Tim Graham <timograham@…>

In 994274ea8315282484bc24aa403b2fd12b5bdeec:

[1.7.x] Fixed #22029 -- Removed obsolete advice on registering migrate/syncdb signal handlers.

All signals should now be registered in AppConfig.ready().

Thanks un33k for the report.

Backport of 5233b36693 from master

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