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

rm cached attributes #15429

Merged
merged 1 commit into from Jun 11, 2014
Merged

rm cached attributes #15429

merged 1 commit into from Jun 11, 2014

Conversation

@sgrif
Copy link
Contributor

sgrif commented May 30, 2014

The original patch that added this concept can be found here. The current default behavior is to cache everything except serialized columns, unless the user specified otherwise. If anyone were to specify otherwise, many types would actually be completely broken. Still, the method is left in place with a deprecation warning in case anyone is actually still calling this method.

@rafaelfranca
rafaelfranca reviewed May 30, 2014
View changes
activerecord/lib/active_record/attribute_methods/read.rb Outdated

# Returns the attributes which are cached. By default time related columns
# with datatype <tt>:datetime, :time, :date</tt> are cached.
def cached_attributes

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca May 30, 2014

Member

We need to deprecate this one too.

@rafaelfranca
rafaelfranca reviewed May 30, 2014
View changes
activerecord/lib/active_record/attribute_methods/read.rb Outdated
end

# Returns +true+ if the provided attribute is being cached.
def cache_attribute?(attr_name)

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca May 30, 2014

Member

And this one

This comment has been minimized.

Copy link
@sgrif

sgrif May 30, 2014

Author Contributor

Updated

@rafaelfranca
Copy link
Member

rafaelfranca commented May 30, 2014

@jeremy WDYT?

@sgrif
Copy link
Contributor Author

sgrif commented May 30, 2014

Note: This does not change behavior for anyone who has not called cache_attributes on a model.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 6, 2014

@jeremy Ping!

@sgrif
Copy link
Contributor Author

sgrif commented Jun 7, 2014

@rafaelfranca @jeremy Any more feedback on this? Need this for what I'm working on.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 9, 2014

@rafaelfranca
Copy link
Member

rafaelfranca commented Jun 9, 2014

For me it is :shipit: but maybe @jeremy is using it for something or he knows some use case that we are not aware of. I'll try to get him reviewing it.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 9, 2014

Thank you! 😄

@jeremy
jeremy reviewed Jun 10, 2014
View changes
activerecord/lib/active_record/attribute_methods/read.rb Outdated
ActiveSupport::Deprecation.warn(<<-MESSAGE.strip_heredoc)
`cache_attributes` no longer has any effect. The default behavior has changed
to cache all attributes by default. There is no down-side to caching type
casting, and not caching mutable types can lead to unexpected behavior.

This comment has been minimized.

Copy link
@jeremy

jeremy Jun 10, 2014

Member

"Calling cache_attributes is no longer necessary. All attributes are cached."

The last sentence talking about downsides makes sense to Rails implementors, not app developers.

@jeremy
jeremy reviewed Jun 10, 2014
View changes
activerecord/lib/active_record/attribute_methods/read.rb Outdated
end
alias cached_attributes cache_attributes
alias cache_attribute? cache_attributes

This comment has been minimized.

Copy link
@jeremy

jeremy Jun 10, 2014

Member

It's odd to get a cache_attributes deprecation when you call cached_attributes or cache_attribute?

@sgrif
Copy link
Contributor Author

sgrif commented Jun 10, 2014

Updated.

@jeremy
Copy link
Member

jeremy commented Jun 10, 2014

👍

The original patch that added this concept can be found
[here](https://web.archive.org/web/20090601022739/http://dev.rubyonrails.org/ticket/9767).
The current default behavior is to cache everything except serialized
columns, unless the user specified otherwise. If anyone were to specify
otherwise, many types would actually be completely broken. Still, the
method is left in place with a deprecation warning in case anyone is
actually still calling this method.
rafaelfranca added a commit that referenced this pull request Jun 11, 2014
rm cached attributes
@rafaelfranca rafaelfranca merged commit 70b931f into rails:master Jun 11, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
sgrif added a commit to sgrif/protected_attributes that referenced this pull request Jun 14, 2014
@sgrif sgrif deleted the sgrif:sg-rm-cached-attributes branch Jun 14, 2014
@samwgoldman
Copy link

samwgoldman commented Aug 6, 2014

Sorry, I know this is old, but I am confused by this change. Why was it necessary? The comments indicate that many types would be broken, but which ones, and how? Why should string-valued attributes be cached?

zzak added a commit that referenced this pull request Aug 20, 2014
Remove cached attributes from Configuring guide [ci skip]

This was removed in #15429
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.