Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forcing null on translation fields seems inappropriate #144

Open
deschler opened this issue Feb 18, 2013 · 15 comments

Comments

@deschler
Copy link
Owner

commented Feb 18, 2013

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.

@zlorf

This comment has been minimized.

Copy link
Collaborator

commented Feb 18, 2013

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.

@zlorf

This comment has been minimized.

Copy link
Collaborator

commented Feb 18, 2013

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).

@acdha

This comment has been minimized.

Copy link

commented Feb 20, 2013

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).

@zlorf

This comment has been minimized.

Copy link
Collaborator

commented Mar 16, 2013

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 '').

@deschler

This comment has been minimized.

Copy link
Owner Author

commented Mar 17, 2013

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.

@zlorf

This comment has been minimized.

Copy link
Collaborator

commented Mar 17, 2013

So... Shall we make a revolution?

@deschler

This comment has been minimized.

Copy link
Owner Author

commented Mar 19, 2013

+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. :)

@zlorf

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2013

#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).

@umazalakain

This comment has been minimized.

Copy link

commented Apr 23, 2013

Any update on this issue?

@zlorf

This comment has been minimized.

Copy link
Collaborator

commented Apr 23, 2013

How referenced issue is connected with this?

@umazalakain

This comment has been minimized.

Copy link

commented Apr 23, 2013

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 ;-)

@zlorf

This comment has been minimized.

Copy link
Collaborator

commented Apr 23, 2013

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.

@umazalakain

This comment has been minimized.

Copy link

commented Apr 23, 2013

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.

@gabn88

This comment has been minimized.

Copy link

commented Nov 15, 2015

@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 :)

@yerihyo

This comment has been minimized.

Copy link

commented Oct 31, 2016

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,)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.