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

Fixes a problem with dup and instance variables #352

Merged
merged 1 commit into from Nov 16, 2014

Conversation

Projects
None yet
6 participants
@hypomodern
Contributor

hypomodern commented May 29, 2014

Fixes a problem where after dup the dup'd model and the original model share a translation instance, which means that if you mutate a translated field on the dup and save it, the original becomes a clone of the dup.

We were running in to this in our efforts to use spree, so there's at least one major implementation that is effected.

Thanks in advance!

Fixes a problem where after dup the dup'd model and the original mode…
…l share a translation instance, which means that if you mutate a translated field on the dup and save it, the original becomes a clone of the dup.
@hypomodern

This comment has been minimized.

Show comment
Hide comment
@hypomodern

hypomodern May 30, 2014

Contributor

I'm not sure why the travis build failed :(. I don't have all of those platforms available, but the test suite passes locally for me in a variety of rubies.

Contributor

hypomodern commented May 30, 2014

I'm not sure why the travis build failed :(. I don't have all of those platforms available, but the test suite passes locally for me in a variety of rubies.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama May 30, 2014

Contributor

Yeah something strange is going on, locally the tests pass sometimes and fail other times for me. The failures on Rails 4.1 may be fixed in #353 if we can make the fix backwards compatible.

Contributor

shioyama commented May 30, 2014

Yeah something strange is going on, locally the tests pass sometimes and fail other times for me. The failures on Rails 4.1 may be fixed in #353 if we can make the fix backwards compatible.

@brauliomartinezlm

This comment has been minimized.

Show comment
Hide comment
@brauliomartinezlm

brauliomartinezlm Jul 21, 2014

After a lot of debugging I realized I came across with the same issue as @hypomodern. This issue makes the gem mess with a LOC in friendly_id gem and I'm having problems using them together, @shioyama can I help on anything to make some progress on this matter?

Thanks a lot in advance!

brauliomartinezlm commented Jul 21, 2014

After a lot of debugging I realized I came across with the same issue as @hypomodern. This issue makes the gem mess with a LOC in friendly_id gem and I'm having problems using them together, @shioyama can I help on anything to make some progress on this matter?

Thanks a lot in advance!

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Jul 23, 2014

Contributor

@brauliomartinezlm thanks for confirming, that bumps the priority of this. I'll look into it on the weekend and merge if there are no issues.

Contributor

shioyama commented Jul 23, 2014

@brauliomartinezlm thanks for confirming, that bumps the priority of this. I'll look into it on the weekend and merge if there are no issues.

@brauliomartinezlm

This comment has been minimized.

Show comment
Hide comment
@brauliomartinezlm

brauliomartinezlm Jul 23, 2014

Thanks a lot!

Just to give further feedback about this, I bumped to this PR in the app I'm working with (which makes an extensive use of globalize and friendly_id) and everything worked out correctly. Didn't see any colateral issues by applying it. I'll try to think about this issue on myself during the weekend to see if I can come up with something, because although it fixes the issue I'm not sure if it's the right thing to clear the memoized value here or a refactor is needed not to share the adapter (I'm a newbie with globalize so maybe this PR is actually the way to go, just not sure).

Let me know if I can help with something, thanks again!

brauliomartinezlm commented Jul 23, 2014

Thanks a lot!

Just to give further feedback about this, I bumped to this PR in the app I'm working with (which makes an extensive use of globalize and friendly_id) and everything worked out correctly. Didn't see any colateral issues by applying it. I'll try to think about this issue on myself during the weekend to see if I can come up with something, because although it fixes the issue I'm not sure if it's the right thing to clear the memoized value here or a refactor is needed not to share the adapter (I'm a newbie with globalize so maybe this PR is actually the way to go, just not sure).

Let me know if I can help with something, thanks again!

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Jul 27, 2014

Contributor

@brauliomartinezlm ah, sorry didn't have time to look into this over the weekend. @parndt any thoughts on this?

Contributor

shioyama commented Jul 27, 2014

@brauliomartinezlm ah, sorry didn't have time to look into this over the weekend. @parndt any thoughts on this?

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Jul 28, 2014

Member

I'd be thrilled for @brauliomartinezlm to come up with a refactored solution rather than clearing values. However, if that doesn't work then I guess we'll just merge this PR.

Member

parndt commented Jul 28, 2014

I'd be thrilled for @brauliomartinezlm to come up with a refactored solution rather than clearing values. However, if that doesn't work then I guess we'll just merge this PR.

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Aug 26, 2014

Any progress on finding another solution to this?

JDutil commented Aug 26, 2014

Any progress on finding another solution to this?

parndt added a commit that referenced this pull request Nov 16, 2014

Merge pull request #352 from hypomodern/fix-dup-translation
Fixes a problem with dup and instance variables

@parndt parndt merged commit a074ca3 into globalize:master Nov 16, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt
Member

parndt commented Nov 16, 2014

@JDutil @hypomodern merged it

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Nov 19, 2014

@parndt could we get a new gem release including this please?

JDutil commented Nov 19, 2014

@parndt could we get a new gem release including this please?

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Nov 20, 2014

Member

@JDutil seems reasonable; can you see anything else in the change history that would cause any problems for 4.0.x?

Member

parndt commented Nov 20, 2014

@JDutil seems reasonable; can you see anything else in the change history that would cause any problems for 4.0.x?

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Nov 21, 2014

@parndt I don't think so, but to be honest I'm not very familiar with this libraries codebase. Only changes that look potentially bad are these Thread changes v4.0.2...masterdiff-0df2502195bd5448967f47dc781b4b96L55 but I'm not really clear on the context of it.

JDutil commented Nov 21, 2014

@parndt I don't think so, but to be honest I'm not very familiar with this libraries codebase. Only changes that look potentially bad are these Thread changes v4.0.2...masterdiff-0df2502195bd5448967f47dc781b4b96L55 but I'm not really clear on the context of it.

@parndt

This comment has been minimized.

Show comment
Hide comment
@parndt

parndt Nov 25, 2014

Member

@JDutil @hypomodern version 4.0.3 is released with this code, thanks!

I hope Spree are able to contribute other useful changes and improvements in future! ❤️

Member

parndt commented Nov 25, 2014

@JDutil @hypomodern version 4.0.3 is released with this code, thanks!

I hope Spree are able to contribute other useful changes and improvements in future! ❤️

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil commented Nov 25, 2014

Thank you @parndt

@JDutil

This comment has been minimized.

Show comment
Hide comment
@JDutil

JDutil Nov 25, 2014

@hypomodern after updating spree_i18n to globalize 4.0.3 I'm still receiving validation error for spree-contrib/spree_i18n#386 did you make changes there as well to fix the problem?

JDutil commented Nov 25, 2014

@hypomodern after updating spree_i18n to globalize 4.0.3 I'm still receiving validation error for spree-contrib/spree_i18n#386 did you make changes there as well to fix the problem?

@nskeip

This comment has been minimized.

Show comment
Hide comment
@nskeip

nskeip Mar 28, 2015

@hypomodern Thank you very much!

nskeip commented Mar 28, 2015

@hypomodern Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment