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

Don't create translation record for default_locale on save #328

Closed

Conversation

scarfacedeb
Copy link
Member

I want to allow users to create localized pages for a specific locale, excluding default locale.
That way I can have a czech-specific page only on the czech version of a site.
But I can't do that because when I'm trying to save a new Page record with a Page::Translation association (locale == :cz), rails gives me a validation error, because globalize has added another association with locale == :en.

I'm using rails_admin and its default field for has_many associations.

I think that my problem starts inside save method.

      def save(*)
        Globalize.with_locale(read_attribute(:locale) || I18n.default_locale) do
          super
        end
      end

In particular, this part in read_attribute:

        if name == :locale
          self.try(:locale).presence || self.translation.locale

Which leads to translation method:

translation_for(::Globalize.locale)

As a result, translation_for builds an empty translation for default ::Globalize.locale locale.


I think that read_attribute(:locale) could skip creating default translation record, because in that particular case it only uses it to get its locale, that is equal to Globalize.locale anyway.
I ran rake test on this branch and on the branch with merged before_callbacks (it's called combined) and there's no new errors.

@shioyama shioyama added the master label Feb 4, 2014
@shioyama
Copy link
Contributor

Thanks, this is interesting and looks sensible. I seem to recall stumbling on a similar problem with translation_for where it was creating an empty translation that wasn't needed... so I'd like to merge this. But I'm still a bit fuzzy on the details of how it's happening in your case. It would be nice to have a failing test, isolated from the specifics of the gem(s) you're using. Could you add that? Something so that we don't accidentally undo this in the future.

@parndt any thoughts?

@scarfacedeb
Copy link
Member Author

I added a couple of tests. They roughly illustrate my problem.

In particular, when I try to save Artwork record with a single Artwork::Translation association with locale == :de (when default locale is equal to :en), it adds another translation for default locale for my record. (so, artwork.translations.count == 2, instead of 1)

Also, if you add any validation into your Translation model, you'll get validation error on save, because you won't be able to save empty translation record for default locale. (This is what I've tried to describe in my first message)

@shioyama
Copy link
Contributor

Really sorry that we're behind on responding to this and other PRs, I've just pushed out 4.0.1 with your change to with_translations in #336. Will try to get to this one soon.

@jirikolarik
Copy link

I had the same problem and there is mini thread #195 #184 about default translation behavior and it's not a bug, it's a feature. So I made micro gem globalize-missing which will allow you save translation without default locale.

@scarfacedeb scarfacedeb force-pushed the default_locale_in_save branch 2 times, most recently from 33c41c7 to 2d9c8b6 Compare October 27, 2014 20:02
@scarfacedeb
Copy link
Member Author

I can't stress enough how sorry I am for such a long pause, but I finally have some time to finish or close my pull requests.
I rebased the current one on the latest master.

@jirikolarik I see your point, but #195 doesn't cover all of the issues.

In my particular case, I want to save the record with translation for not-default locale (:de in test).
But in the master branch, it would also create a translation for default locale (:en in test).
And I think, that it should allow to skip default locale if it's needed.

Although, now it won't create a translation for default locale on save at all, if we didn't assign any translated attribute. (This is almost exactly the same issue as #195)
But I think, that it's a desired behavior, because it doesn't need to have a translation record at the moment and it would create it when you'd assign any of the translated attributes. (I added unit test for that in the e0c31d8)

@shioyama could you take at look at it again, please?

@shlima
Copy link

shlima commented Oct 2, 2015

+1

@jerefrer
Copy link

jerefrer commented Nov 9, 2016

Any news on this ?

I'm having the same issue and I have trouble imagining how there could not be an army of people effected by this behavior.
Also it took me about an hour and a half of painful debugging and questioning to arrive to the conclusion that something might be wrong with globalize itself.

Looking at the Travis failure it seems to me that it's related to some testing environment quirks and not to the actual commits discussed here.

What do you think ?

EDIT: I was just thinking that maybe some (foolish) people would actually rely on this behavior in their application. I that case maybe adding a configuration option like auto_build_empty_translation_for_default_locale that defaults to true would be the solution.

EDIT 2: I'm realizing that the reason I have to fight globalize might be that empty translations are supposed to happen, as pardnt was saying in another issue. I can't take more time on this for today, I'll update with future realizations tomorrow.

@jtomaszewski
Copy link

jtomaszewski commented Nov 20, 2016

@jerefrer , I'm going through all of it now as well.

I'm realizing that the reason I have to fight globalize might be that empty translations are supposed to happen, as pardnt was saying in another issue. I can't take more time on this for today, I'll update with future realizations tomorrow.

You mean this comment #195 (comment) ?

Well, we should start with that question then: Do we want to create a Translation object for a default_locale even if we don't use it at all (f.e. it doesnt have any attributes set)?

IMHO, I don't see any reasons we should do that. (Of course we can allow it to be configurable with some param, to keep backwards compability)
Is this default translation object being saved to database atm?

My cases:

  • I want to create a record which doesn't have any translations
  • I want to create a record which has translations, but none of them are in default_locale

@jtomaszewski
Copy link

@scarfacedeb 's fix is working and not really messing up much with the specs. I rebased it in #578 so you can see it. How about somebody gives us a decision what we're gonna do finally about it? ;)

jtomaszewski added a commit to jtomaszewski/globalize that referenced this pull request Mar 31, 2017
Ceda added a commit to blueberryapps/globalize that referenced this pull request Aug 2, 2018
…lation-on-save

Don't create translation record for default_locale on save (globalize#328)
Ceda added a commit to blueberryapps/globalize that referenced this pull request Aug 2, 2018
…lation-on-save

Don't create translation record for default_locale on save (globalize#328)
michalgritzbach pushed a commit to blueberryapps/globalize that referenced this pull request Aug 3, 2018
javierm added a commit to javierm/consul that referenced this pull request Oct 11, 2018
Globalize creates a translation for the current locale, and the only way
I've found to change this behaviour is to monkey-patch it.

See also the following convertations:

globalize/globalize#328
globalize/globalize#468
globalize/globalize#578
https://github.com/shioyama/mobility/wiki/Migrating-from-Globalize#blank-translations
javierm added a commit to AyuntamientoMadrid/consul that referenced this pull request Oct 11, 2018
Globalize creates a translation for the current locale, and the only way
I've found to change this behaviour is to monkey-patch it.

See also the following convertations:

globalize/globalize#328
globalize/globalize#468
globalize/globalize#578
https://github.com/shioyama/mobility/wiki/Migrating-from-Globalize#blank-translations
javierm added a commit to AyuntamientoMadrid/consul that referenced this pull request Oct 18, 2018
Globalize creates a translation for the current locale, and the only way
I've found to change this behaviour is to monkey-patch it.

The original code uses `translation.locale` instead of
`Globalize.locale`. Since `translation.locale` loads the translation
with empty attributes. It both makes the record invalid if there are
validations and it makes it almost impossible to create a record with
translations which don't include the current locale.

See also the following convertations:

globalize/globalize#328
globalize/globalize#468
globalize/globalize#578
https://github.com/shioyama/mobility/wiki/Migrating-from-Globalize#blank-translations
javierm added a commit to javierm/consul that referenced this pull request Oct 22, 2018
Globalize creates a translation for the current locale, and the only way
I've found to change this behaviour is to monkey-patch it.

The original code uses `translation.locale` instead of
`Globalize.locale`. Since `translation.locale` loads the translation
with empty attributes. It both makes the record invalid if there are
validations and it makes it almost impossible to create a record with
translations which don't include the current locale.

See also the following convertations:

globalize/globalize#328
globalize/globalize#468
globalize/globalize#578
https://github.com/shioyama/mobility/wiki/Migrating-from-Globalize#blank-translations
decabeza pushed a commit to consuldemocracy/consuldemocracy that referenced this pull request Nov 5, 2018
Globalize creates a translation for the current locale, and the only way
I've found to change this behaviour is to monkey-patch it.

The original code uses `translation.locale` instead of
`Globalize.locale`. Since `translation.locale` loads the translation
with empty attributes. It both makes the record invalid if there are
validations and it makes it almost impossible to create a record with
translations which don't include the current locale.

See also the following convertations:

globalize/globalize#328
globalize/globalize#468
globalize/globalize#578
https://github.com/shioyama/mobility/wiki/Migrating-from-Globalize#blank-translations
decabeza pushed a commit to consuldemocracy/consuldemocracy that referenced this pull request Jan 21, 2019
Globalize creates a translation for the current locale, and the only way
I've found to change this behaviour is to monkey-patch it.

The original code uses `translation.locale` instead of
`Globalize.locale`. Since `translation.locale` loads the translation
with empty attributes. It both makes the record invalid if there are
validations and it makes it almost impossible to create a record with
translations which don't include the current locale.

See also the following convertations:

globalize/globalize#328
globalize/globalize#468
globalize/globalize#578
https://github.com/shioyama/mobility/wiki/Migrating-from-Globalize#blank-translations
@stale
Copy link

stale bot commented Dec 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Dec 6, 2019
@stale stale bot closed this Dec 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants