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

Changed concatenate operator to keep the "localized_label" string unmodified. #805

Merged
merged 2 commits into from Feb 23, 2012

Conversation

flexoid
Copy link
Contributor

@flexoid flexoid commented Feb 18, 2012

Required for localizer caching.
Using << instead of + causes "required_string" merge with string in cache every request and we have labels like "Title******************" after several requests.

@justinfrench
Copy link
Member

Weird! Either operator should be safe. Have you got any idea where the root cause is? Happy to merge this in, but would also love to plug the real cause and add spec coverage against a regression.

@flexoid
Copy link
Contributor Author

flexoid commented Feb 23, 2012

The problem is because << changes the object on its left hand side. And string from the localized_label is the same object that stored in the cache. So, using << we change value in the cache every time we execute
((localized_label || humanized_method_name) << requirement_text).html_safe, what means every request to the page with this label.
+ solves this problem because it leaves localized_label and, respectively, value in cache, unchanged.

justinfrench added a commit that referenced this pull request Feb 23, 2012
Changed concatenate operator to keep the "localized_label" string unmodified.
@justinfrench justinfrench merged commit 4463c41 into formtastic:master Feb 23, 2012
@justinfrench
Copy link
Member

Merged in, thanks.

@justinfrench
Copy link
Member

@flexoid Looks like this failed the build under 1.8.7 and ree. Can you take a look for any 1.9-specific code, check out the fail and submit a patch please? http://travis-ci.org/#!/justinfrench/formtastic/builds/730924

@justinfrench
Copy link
Member

Looks like some 1.9-only hash syntax.

justinfrench added a commit that referenced this pull request Feb 24, 2012
@flexoid
Copy link
Contributor Author

flexoid commented Feb 24, 2012

Sorry, just forgot about compatibility. Thanks for fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants