Open deschler opened 11 years ago
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.
So... Shall we make a revolution?
+1 for a revolution, but let's first gather some facts. From what i understand, these issues are all related:
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. :)
Any update on this issue?
How referenced issue is connected with this?
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,
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,)
We currently force
null=True
on all translation fields (except forBooleanField
which has no such option) through theTranslationField
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: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.