Opened 16 months ago

Closed 5 months ago

Last modified 3 months ago

#21498 closed Bug (fixed)

Changing locale causes migrations autodetector to make unnecesary changes

Reported by: foonicorn Owned by: andrewgodwin
Component: Migrations Version: master
Severity: Release blocker Keywords:
Cc: info@…, loic@…, andrew@…, tomas.kostrhun@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

A migration is created after fields like help_text changed, although this is not relevant for the database. This also occurs when i18n is used and the translation changes.

Change History (24)

comment:1 Changed 16 months ago by MarkusH

  • Cc info@… added
  • Needs documentation unset
  • Needs tests unset
  • Owner set to MarkusH
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 follow-up: Changed 16 months ago by loic84

FWIW I don't think it's a good idea to start blacklisting / whitelisting parameters, especially with custom fields in mind.

comment:3 in reply to: ↑ 2 Changed 16 months ago by MarkusH

I'm going to implement it in a way that fields can define their own non-db-relevant arguments.

comment:4 Changed 16 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 16 months ago by loic84

  • Cc loic@… added

comment:6 Changed 16 months ago by andrewgodwin

I'd rather we included every single field - white/blacklisting fields is dangerous. What if a field subclass redefines help_text? What if it uses it to help populate ORM objects? I'd like to mark this WONTFIX.

comment:7 Changed 16 months ago by loic84

+1, I'm particularly concerned by the fate of validators which were blacklisted in the tentative patch presented on IRC.

Also with the upcoming VirtualField we will be blurring the lines more and more between the ORM and the underlying schema.

However there seems to be an valid issue here, i18n locale shouldn't trip the autodetector, I haven't had time to look into it but I suspect this is due to the fix for #21008.

comment:8 Changed 16 months ago by MarkusH

I think we should find a solution for this: imaging your translators change the translation for a help text. All the time they do one developer will end up with such a new migration.

The way I implemented it allows fields to redefine their attributes. It's a prove of concept: https://github.com/Markush2010/django/tree/ticket21498

comment:9 Changed 14 months ago by andrewgodwin

I agree that the locale shouldn't trip the autodetector, but changing help_text unfortunately has to (we just can't guarantee it's not useful at database or field runtime).

Perhaps we should force a certain locale while running the autodetector?

comment:10 Changed 14 months ago by andrewgodwin

  • Cc andrew@… added

comment:11 Changed 14 months ago by andrewgodwin

  • Summary changed from Unnecessary migrations after nonrelevat fields changed to Changing locale causes migrations autodetector to make unnecesary changes

comment:12 Changed 14 months ago by loic84

To solve this we may need a better way to handle Promises, maybe we could make them "deconstructible"?

comment:13 Changed 14 months ago by andrewgodwin

Well, the serializer currently runs any Promise through force_text before saving it, but the autodetector doesn't serialize anything before it compares. To be honest, if we could make Promises comparable (and make them evaluate on eq) that would solve it.

comment:14 Changed 9 months ago by andrewgodwin

  • Owner changed from MarkusH to andrewgodwin
  • Severity changed from Normal to Release blocker

Claiming this to fix it tonight, as it just got way worse now we have migrations for contrib apps (upping to release blocker due to that)

comment:15 Changed 9 months ago by Andrew Godwin <andrew@…>

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

In d359647715bccffd7cfc5eb8bcf5edb5c714fb00:

Fixed #21498: Don't use a fallback language if you're en-us.

comment:16 Changed 9 months ago by andrewgodwin

Oddly, no fix was needed on the stable/1.7.x branch, as it did not show the symptom in my tests; this appears to have been fixed by a the new autodetector code and then broken by https://github.com/django/django/commit/a5f6cbce07b5f3ab48d931e3fd1883c757fb9b45.

If someone can get the problem on the stable/1.7.x branch please reopen.

comment:17 Changed 5 months ago by ChillarAnand

  • Resolution fixed deleted
  • Status changed from closed to new

comment:18 Changed 5 months ago by charettes

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

@ChillarAnand, please don't re-open without providing a valid reason.

Did you manage to reproduce the issue on the stable/1.7.x branch?

comment:19 Changed 3 months ago by tcx

I can reproduce it on 1.7.2.

Started the project with

LANGUAGE_CODE = 'cs'
LANGUAGES = ( ('cs', 'Czech'), ('en', 'English'), )
USE_I18N = True

all model fields with verbose_name in Czech like

from django.db import models
from django.utils.translation import ugettext_lazy as _

class Book(models.Model):
    name = models.CharField(_(u'Název'), max_length=255)

initial migration

('name', models.CharField(max_length=255, verbose_name='N\xe1zev')),

after I've added en locale and translated "Název" as "Title" makemigrations shows

migrations.AlterField(
    model_name='book',
    name='name',
    field=models.CharField(max_length=255, verbose_name='Title'),
    preserve_default=True,
),

when I import _ in models.py just as ugettext (not ugettext_lazy) problem disappears.

comment:20 Changed 3 months ago by tcx

  • Cc tomas.kostrhun@… added

comment:21 follow-ups: Changed 3 months ago by timgraham

Hi Tomas, can you test with Django's master branch? I believe that issue might be related to/fixed by f7c287fca9c9e6370cc88d1457d3ed9466703687.

comment:22 in reply to: ↑ 21 Changed 3 months ago by MarkusH

Replying to timgraham:

Hi Tomas, can you test with Django's master branch? I believe that issue might be related to/fixed by f7c287fca9c9e6370cc88d1457d3ed9466703687.

It's exactly the case I constructed in https://github.com/django/django/pull/3838#issuecomment-68784330

comment:23 Changed 3 months ago by claudep

In Django 1.8, you *might* obtain a new migration just after having upgraded from 1.7 (if the existing migration uses a translated string), but then the translations won't affect migrations any longer.

comment:24 in reply to: ↑ 21 Changed 3 months ago by tcx

Replying to timgraham:

Hi Tomas, can you test with Django's master branch? I believe that issue might be related to/fixed by f7c287fca9c9e6370cc88d1457d3ed9466703687.

Hello Tim, yes, django trunk shows "No changes detected" so it's OK.

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