django-modeltranslation
django-modeltranslation copied to clipboard
deschler
Forcing null on translation fields seems inappropriate
We currently force null=True on all translation fields (except for BooleanField which has no such option) through the TranslationField constructor. Is it really wise to do so? Shouldn't we better respect the null option of the original field?
The Django docs clearly recommend against setting null for string-based fields:
Avoid using null on string-based fields such as CharField and TextField unless you
have an excellent reason. If a string-based field has null=True, that means it has
two possible values for “no data”: NULL, and the empty string. In most cases, it’s
redundant to have two possible values for “no data;” Django convention is to use
the empty string, not NULL.
Are our fallback values such an excellent reason, are there others?
Changing the behaviour might have an impact on the fallback values and qualifies as a backwards incompatible change.
The question came up again by issue #143.
It's something to think about.
For me nulling is proper - there are some types of fields (like IntegerFIeld) where there is no 'empty value' other than null.
Of course, if we make #143, we won't set null=True on the required fields. But I think it's necessary on non-required ones.
And we may change behaviour to not set null on CharField and TextField (and their subclasses).
It's necessary to allow null this if you want to have a unique index on that field but would like to have more than one record with a blank field (this edge case should be discussed in the Django docs as it's easy for people not to notice with limited test data).
Working with #122 made me change my mind - there are situations when nulling may not be appropriate.
There is a fine looking field property called empty_strings_allowed. Maybe we could check it and don't force a null when empty string are allowed (mainly CharField and TextField)?
It would mean changing line
https://github.com/deschler/django-modeltranslation/blob/required-languages/modeltranslation/fields.py#L97 into:
if not isinstance(self, fields.BooleanField) and not self.empty_strings_allowed
However, this change would break many test (where None is expected for empty fields - with this change it would be '').
empty_strings_allowed looks good, i like the expressiveness of that variable name. :)
The number of broken tests also scared me a bit, it's clearly a backwards incompatible change, people might have coded against the original behaviour. On the other hand we are not at 1.0 yet and the change seems to be the right thing.
+1 for a revolution, but let's first gather some facts. From what i understand, these issues are all related:
- #143 (where we have some code in the required-languages branch already)
- #163 (which gives deeper and valueable insights about this issue and has code that takes a different approach)
- And then there's a bug report at the mailing list which has been confirmed by another user, but we don't have a test or detailed example which shows the actual problem. There's a simple test in the test-unique-nullable-fields branch. Since it passes, it's either wrong or lacking. I have a vague feeling that it might be related to the admin integration, which had some workarounds that have been removed during the old rule3 removal.
Unfortunately i'm currently swamped with work and probably won't be able to get my head around the bigger picture before next month. That being said, you and @wrwrwr seem to be deeper into this issue than myself already, so you have all my blessings to make the revolution happen. :)
#163 is related, but it doesn't present different approach to the same problem, but rather different problem. So to say, revolution wouldn't affect that problem, because in #163 there is a CharField which is nullable from start, and here we are considering non-nullable fields (with nullable translation fields).
Because of using django-modeltranslation and django-markitup together I must define a default value explicitly (#122). I think that makes modeltranslation not to fall back to the default languages translation.
I haven't investigated it too much, I only referenced here for future doc. Then I reviewed the thread and noticed that conversations ended a month ago and no commit working on this issue was made so I thought I might ask how are things going ;-)
As you noticed, things are not going anyhow. :( I'm very busy right now, and I suspect that @deschler too.
However, look at the https://github.com/deschler/django-modeltranslation/blob/master/modeltranslation/fields.py#L215. If default value is set to empty string, it should not stop MT from falling back to default language.
Good luck with your business! ;-)
I suppose that it doesn't stop to MT from falling back to the default value but I think that https://github.com/deschler/django-modeltranslation/blob/master/modeltranslation/fields.py#L218 is returning the original fields default value: an empty string.
@unaizalakain That is exactly what is happening. I first used null=True, blank=True on my charfields, but I didn't like it after having some issues with it (especially in combination with the REST api) and also conceptually with having two 'empty' values (null and ''). Therefore I have changed the fields to NOT include null=True and blank=True, but now I spotted some problems with the translations.
A.k. enter a field with a active language, then login with a different language and the field shows as empty, even with MODELTRANSLATION_ENABLE_FALLBACKS = True and MODELTRANSLATION_AUTO_POPULATE = 'all'
This is definitely unexpected.
Nevermind! I forgot to include MODELTRANSLATION_ENABLE_FALLBACKS = True and the FALLBACK_LANGUAGES! Thanks for this great app :)
Any update or decision on this issue?
I really want to make TranslationField not nullable for my CharField variables, there seems to be no easy way to configure TranslationField.null=False.
A go-around solution could not be easily found on the web, and I had to come up a solution of my own. For anyone who is suffering from any similar problem, below is my temporary solution to this issue. Please remove this post if not proper for this page.
Few points I want to mention with the solution below,
- Model(e.g. Place)'s 'default' value need to be set explicitly to force default value for translated fields
- override_translation_field_nullability function not necessary if you want to simply force null=False for your fields.
- original post for possible future updates: http://yerihyo.wikidot.com/blog:85
models.py
from django.db import models
class Place(models.Model):
name = models.CharField(max_length=255, default="",)
translation.py
from modeltranslation.translator import translator, TranslationOptions, register
from .models import Place
def override_translation_field_nullability(self, field, translation_field,):
from django.db.models import fields
field_instance = self.model._meta.get_field(field)
if isinstance(field_instance,fields.CharField):
translation_field.null = False
class PlaceTranslationOptions(TranslationOptions):
fields = ('name',)
def add_translation_field(self, field, translation_field):
override_translation_field_nullability(self,field,translation_field)
rv = super(PlaceTranslationOptions,self).add_translation_field(field, translation_field)
return rv
translator.register(Place, PlaceTranslationOptions,)
