Opened 9 years ago
Closed 9 years ago
#27832 closed Bug (invalid)
contenttypes migration not following the doc on hints naming
| Reported by: | Hervé Cauwelier | Owned by: | Hervé Cauwelier |
|---|---|---|---|
| Component: | contrib.contenttypes | Version: | 1.10 |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Accepted | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
Hi,
The second migration for contenttypes provides a RunPython operation: https://github.com/django/django/blob/master/django/contrib/contenttypes/migrations/0002_remove_content_type_name.py#L33
As you can see on the highlighted line, the hint is named "model_name".
But the doc suggests the key should be "model": https://docs.djangoproject.com/en/1.10/topics/db/multi-db/#allow_migrate
Not a big issue but I ran into this when writing the "allow_migrate" method of a database router.
Change History (7)
comment:1 by , 9 years ago
| Component: | Migrations → contrib.contenttypes |
|---|---|
| Severity: | Normal → Release blocker |
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 9 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
I guess it's a low-hanging fruit for a newcomer like me, assigning it to myself.
comment:4 by , 9 years ago
| Severity: | Release blocker → Normal |
|---|
Now that I see that migration was added in Django 1.8, I'm not sure if there's much value in fixing this. 1.8/1.9 only receive security fixes at this point. Does the fix have some value if it's changed in later versions of Django?
Separately, we might consider a strategy for squashing contrib app migrations so we can eventually remove obsolete migrations like that one.
comment:5 by , 9 years ago
I always assumed you meant Django 1.10 and later. Django 1.8 and 1.9 happily lived with this typo. :-)
So we could just leave both keys without a deprecation path and that migration will naturally disappear when migrations are squashed?
As for the added value of this fix, it would avoid me to write a special case in my database router when "model_name" is "contenttype".
comment:6 by , 9 years ago
Oops! I've added some print()s in my code to discover it was just a coincidence if the router didn't choke on KeyError. With the patch, it's trigerring another code path or something, let me sort this out first.
comment:7 by , 9 years ago
| Resolution: | → invalid |
|---|---|
| Status: | assigned → closed |
OK I missed the point when hints contains "model_name" and it's extracted and passed as an argument to allow_migration().
So yes indeed it was triggering the code path in my router when "model_name" is defined.
This patch in django just prevented passing a "model_name" argument in the first place, but any name would do, even "foobar".
Closing this ticket, thank you for your time.
For backwards compatibility at this point, I guess we should add
'model'there instead of renaming'model_name'.