Variants select and products search not working any more.

[tvdeyen]

First I thought this is only an issue in the variants select in the "admin new order" view (https://github.com/spree/spree/issues/6146).

After researching I realized that currently the complete product search is broken since Globalize 5 does not set the original value anymore.

So, instead of:

Spree::Product.search(name_cont: 'brand')

we need to call:

Spree::Product.with_translations(I18n.locale).search(translations_name_cont: 'brand')

This means, we have to decorate all admin controllers AND deface all admin form fields that contain translated values :scream: This seams way to hackish for me.

Maybe it's time to merge the globalize feature into spree core? :trollface:

Seriously, we currently force all spree_i18n users to also have globalize in their project anyway, although they don't always need it. spree_i18n was once a repository for translation files only, not for multi language stores.

Since this "fix" needs to be deeply integrated into spree_core, I think it is better integrated into the core.

Spree has a global foot print by now :raised_hands: and is used all over the world. Does the globalization feature harm existing users? I don't think so. IMHO it is more harmful to maintain an extension that patches so many core files, like we need to do now.

So, let's split up spree_i18n into two pieces:

1. The translation files stay where they always where.
2. The globalization feature gets merged into core.

What do you think?

[JDutil]

I agree that i18n should probably just be part of spree itself for proper support, but everyone else pushes back against that when I suggest it... Perhaps splitting into two pieces won't meet as much resistance, but I have my doubts. Will pass this along to get some other peoples feedback.

[GeekOnCoffee]

My recollection is that the only reason that i18n was separate was to make contributing translations easier

[tvdeyen]

Yeah @GeekOnCoffee totally right!

@JDutil let's ask the community :) Let them decide. I think they would be totally fine with it. And since it will be much easier to maintain, the core team should also agree with it. Everything else is just chasing the rabbit.

[JDutil]

Yeah that was definitely the case, but that was also the case for the guides as well which we've merged back in. I'm fine with the split approach of moving globalize into core, and leaving only translations here for more committers to have access accepting translation updates.

[tvdeyen]

Yes, translations are totally fine in spree_i18n, but everything else should be in the core.

[huoxito]

Hey guys please lets consider improving either spree_i18n or globalize before even thinking of moving this into spree core. I'm sure there's a good chance we can come up with a better hack in spree_core or globalize (without having to override every endpoint form here)

[huoxito]

I'm happy to have a real close look at this myself sometime soon

[mamhoff]

Imho, for a large part of the Spree user base, spree-18n is actually part of the core (you can't use it in other languages without it). I agree: maybe move the logic stuff to core, and then improve it lateron... I think moving this to core would make it get the attention it deserves, too. :+1:

[masterkain]

I'm not entirely sure mine is another problem altogether, but I'm having issues with globalize as well on Spree 3.0, localized attributes gets stripped well before hitting the database, for example when I try to create a Taxonomy (or Taxon for instance) in a spec with a basic create(:taxonomy, name: 'test test') the name gets stripped and it goes hitting the database without, bypassing presence validations:

  SQL (0.3ms)  INSERT INTO "spree_taxonomies" ("created_at", "updated_at", "position") VALUES ($1, $2, $3) RETURNING "id"  [["created_at", "2015-03-19 04:07:54.298046"], ["updated_at", "2015-03-19 04:07:54.298046"], ["position", 1]]

#<Spree::Taxonomy:0x007fff4995bbb8> {
  :id         => 1,
  :name       => nil,
  :created_at => Thu, 19 Mar 2015 04:07:54 UTC +00:00,
  :updated_at => Thu, 19 Mar 2015 04:07:54 UTC +00:00,
  :position   => 1
}

Disabling gem 'spree_i18n', github: 'spree/spree_i18n', branch: '3-0-stable' and all is well.

[JDutil]

@masterkain it's the result of the same changed behavior w/Globalize 5. Globalize is only storing data in the translations table now, and not on the main record. Even before you would get data messed up on the main record though as alternate locales would be saved there rather than your default.

[robinboening]

As most of you already said, moving globalize to spree_core and leaving the translation files in spree_i18n would probably the best idea to a) not loose focus on the feature and b) keep the translations accessible by everyone for easy contribution.

:+1:

[tvdeyen]

Another option would be to move the globalize functionality into its own extension ('spree_globalize' for instance).

Not everybody is using the model translations, but want to have the GUI translations. We don't need to translate our products, but a translated backend for our customer.

So why not return in to the originally state of this gem and move globalize out. Either into the core or into its own extension.

But the current state is somehow unsatisfying.

[futhr]

I believe in moving Globalize into its own gem, then its possible to create a spree_traco gem for users that only requires very few locales for translating model data. Keeping the actual model data maintained for 10+ locales within a project requires dedicated staff for that task if you have many products, and those types of companies got cash flow for contribution time to "their future, and dear" spree_globalize gem.

Spree would benefit from having it's core ready for a setup for any language within itself, then I also believe it would result in that the translations would always be 100% translated all time.

[JDutil]

I'm thinking spree_globalize seems to be the way most people are pointing towards from discussions here and offline.

[huoxito]

@tvdeyen any chance you could give a try on spree_i18n/ransack-translations branch please? (see PR above)

If I got this right the issue lies in ransack not knowing anything about globalize, the original ar api e.g. where seems to be working fine so perhaps if we tell ransack about the translations we could fix this in one place only.

About having globalize into spree core .. just please dont. I find spree an extremely complex project (itself and all of its dependencies mokey patching one another) and throwing globalize there wouldn't be fair given most stores will never need to translate on db level (will they?). I'd definitely agree though that having globalize here could be misleading mainly for beginners so yeah splitting globalize and the locales logic here sounds pretty awesome.

[JDutil]

Awesome idea & better yet work @huoxito really excited you dug into this :beers:

[alepore]

:+1: splitting globalize and locales, or just... add instructions on how to copy the locale files to your project. that's pretty trivial and same thing as https://github.com/svenfuchs/rails-i18n#manual-installation

not sure about the merge thing, just want to point out that we still have the N+1 problem on translated models. maybe spree should just accept some little tweaks to better integrate the globalize stuff... for example, a default blank with_translations scope that can be overwritten by spree_i18n... just thinking

[tvdeyen]

@huoxito I will have a look into #532 later today, thanks for patching.

[romul]

About having globalize into spree core .. just please dont.

:+1: globalize is too complicated solution, even in case when you need a few locales, and the most of stores have only one locale. So, the best choice is removing globalize from spree_i18n repository and leave only I18n translations, as it was in early versions.

[tvdeyen]

Ok, since https://github.com/spree-contrib/spree_i18n/pull/532 seems to be stalled, I will open a pull request later that removes Globalize from spree_i18n, so that it doesn't depend on that anymore. Then we should introduce spree_globalize.

[tvdeyen]

The issue that #532 tries to solve could then be handled in spree_globalize

[mamhoff]

@tvdeyen Super, all for it.

[huoxito]

hey just for the record in case anyone needs it, #532 has been fixed on rails master https://github.com/rails/rails/pull/20196. The 4-2-stable PR hasnt been merged yet though https://github.com/rails/rails/pull/20356.

either way also plus one for a separate spree_globalize gem

[tvdeyen]

Nice! Great news. I will provide a PR soon. Anyone wants to create the new repo?

https://github.com/spree-contrib/spree_globalize

[alepore]

:+1: to separate gems, but still have broken searches on the 3.0 admin right? :)

[tvdeyen]

@alepore only if you use the new spree_globalize gem

[alepore]

@tvdeyen i mean, all 3-0-stable users are already using globalize and will need a fix. spree_globalize is a plan for the next version right?

[tvdeyen]

Sure, it's a breaking change. Yes, you're absolutely right, 3-0-stable still needs to be fixed, though.

[tvdeyen]

https://github.com/spree-contrib/spree_i18n/pull/588 Please review and provide Feedback

[JDutil]

@tvdeyen I've created and sent out invites for https://github.com/spree-contrib/spree_globalize