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

Deprecate `serialized_attributes` without replacement #15704

Merged
merged 1 commit into from Jun 14, 2014

Conversation

@sgrif
Copy link
Contributor

sgrif commented Jun 14, 2014

We've stopped using it internally, in favor of polymorphism. So should
you!

@matthewd
matthewd reviewed Jun 14, 2014
View changes
activerecord/lib/active_record/attribute_methods/serialization.rb Outdated
`serialized_attributes` is deprecated, and will be removed in Rails 5.0.
If you need to access the serialization behavior, you can do:

#{self.class.name}.type_for_attribute('foo').type_cast_for_database(value)

This comment has been minimized.

Copy link
@matthewd

matthewd Jun 14, 2014

Member

Isn't this internal API?

@matthewd
matthewd reviewed Jun 14, 2014
View changes
activerecord/lib/active_record/attribute_methods/serialization.rb Outdated
self.serialized_attributes = serialized_attributes.merge(attr_name.to_s => coder)
#{self.class.name}.type_for_attribute('foo').type_cast_from_database(value)
WARNING
Hash[columns.select { |t| t.cast_type.is_a?(Type::Serialized) }.map { |c|

This comment has been minimized.

Copy link
@matthewd

matthewd Jun 14, 2014

Member

Let's go with an each_with_object here, and/or consider memoizing. People obviously should be moving away from it -- that's what the deprecation message is for -- but until then... it used to be super cheap: we probably want to be aware that if people are calling it, they're likely to be doing so with reckless abandon.

We've stopped using it internally, in favor of polymorphism. So should
you!
@sgrif
Copy link
Contributor Author

sgrif commented Jun 14, 2014

@matthewd Updated.

senny added a commit that referenced this pull request Jun 14, 2014
Deprecate `serialized_attributes` without replacement
@senny senny merged commit 69807af into rails:master Jun 14, 2014
1 check was pending
1 check was pending
continuous-integration/travis-ci The Travis CI build is in progress
Details
@senny
Copy link
Member

senny commented Jun 14, 2014

@sgrif 👍

@sgrif sgrif deleted the sgrif:sg-deprecate-serialized branch Jun 14, 2014
zzak added a commit to ActsAsParanoid/acts_as_paranoid that referenced this pull request Aug 29, 2014
prathamesh-sonpatki added a commit to prathamesh-sonpatki/activerecord-jdbc-adapter that referenced this pull request Aug 30, 2014
- rails/rails#15704 has deprecated
  serialized_attributes.
- It will be removed in Rails 5.
prathamesh-sonpatki added a commit to prathamesh-sonpatki/activerecord-jdbc-adapter that referenced this pull request Oct 24, 2014
- rails/rails#15704 has deprecated
  serialized_attributes.
- It will be removed in Rails 5.
kares added a commit to kares/activerecord-jdbc-adapter that referenced this pull request Nov 6, 2014
- rails/rails#15704 has deprecated
  serialized_attributes.
- It will be removed in Rails 5.
@xaviershay
Copy link
Contributor

xaviershay commented Jan 25, 2015

I'm getting this deprecation just by using the serialize DSL method, then calling update_attributes. Is that expected? I don't see anything here about the serialize API as such. On 4.2.0.

@sgrif
Copy link
Contributor Author

sgrif commented Jan 25, 2015

You should not see that deprecation warning from Rails. It's probably a gem causing that issue for you. If you can reproduce the issue using just Rails, please open an issue with a script we can use to reproduce the issue using https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb as a template.

@xaviershay
Copy link
Contributor

xaviershay commented Jan 25, 2015

Ah, found it. For anyone who stumbles across this, pretty sure it was this gem/issue causing it: paper-trail-gem/paper_trail#416

@simi
Copy link
Contributor

simi commented Feb 6, 2015

What about when you need this meta data before table is created (in migration)? You can check our example globalize/globalize#407.

@baweaver
Copy link
Contributor

baweaver commented Feb 12, 2015

Whenever things like this are removed, it would be prudent to add additional details in the deprecation warnings as to how to properly approach the issue. It strikes me as a bit irresponsible to deprecate with no clear migration paths for those that aren't keen on polymorphism.

Whether or not it's better or more correct is irrelevant, a deprecation needs to be explained in more detail so as to provide the rationale behind it. To the core team that may be perfectly clear, but for an outsider to see something like we're using X now because X is better can be confounding.

For anyone who manages to stumble on this wondering what polymorphics are, this may be of assistance:
http://www.gotealeaf.com/blog/understanding-polymorphic-associations-in-rails

Is there a standard approach to deprecation that can be updated to include the rationale behind changes and a bit more meta? It'd provide a lot of value in upgrades and migrations to new versions.

@sgrif
Copy link
Contributor Author

sgrif commented Feb 12, 2015

Polymorphism actually refers to asking an object to do something without caring about how it accomplishes it. This doesn't have anything to do with associations. It's a general programming term. http://en.wikipedia.org/wiki/Polymorphism_%28computer_science%29 This deprecation came from a lot of refactoring work where the rest of Active Record didn't need to care about the internals of what columns were serialized. We instead have attribute assignment work consistently, and delegate any behavior that needs to change for serialized attributes to the serialized type object, which sits where the other types would have lived. Continuing to maintain this method would have caused more problems than it solved. If you need help figuring out the right way to migrate forward, opening a thread on the mailing list might be a good place to start. Coming here and telling me why "I'm doing it wrong" isn't a great way to open a dialog.

@baweaver
Copy link
Contributor

baweaver commented Feb 12, 2015

But should it be the duty of the mailing list / IRC / etc to fill in the blanks? What I'm saying is more information would be helpful, and I'm sorry if you took that as a personal affront, I didn't mean it as such. It just becomes tedious to hunt some of these things down as occasionally a seemingly minor feature can balloon out quickly whether it be from dependencies or your own code.

@sgrif
Copy link
Contributor Author

sgrif commented Feb 12, 2015

But should it be the duty of the mailing list / IRC / etc to fill in the blanks?

If by the fill in the blanks, you mean be a communication channel for us when people have concerns they need to express? Yes. That's exactly what they're for...

What I'm saying is more information would be helpful

What information are you looking for? All you've said is "how could you not give more details this is so wrong". The rationale was that this method didn't feel needed any longer, it was a burden to keep it around. I'm not sure what "additional information" you're looking for. If you have code that depends on it, a better way to start would have been "Hey, I'm doing X and I'm not sure how to move forward", and then this conversation would be about what you could be doing in your code instead, rather than arguing about deprecation warnings.

@chancancode
Copy link
Member

chancancode commented Feb 12, 2015

@baweaver perhaps to put it another way, there are two kind of deprecations: with and without replacement.

The former refers to cases there is still a legitimate need for the thing being deprecated, in which case we will try to recommend migration options in the message itself or in the upgrade guides.

The latter is for things that we recognized as "it was a bad idea afterall", or otherwise no longer necessary because of other improvements, or things that we can no longer support. (e.g. new Ruby versions has a similar thing, or we switched the implementation so supporting that is not feasible).

This particular implementation falls under the latter bucket. We'll try (within reasons) to associate these information to the commits, but like Sean said, for this case it seems appropriate to use the mailing list for this.

@KelseyDH
Copy link

KelseyDH commented Mar 4, 2015

Could somebody outline how existing applications using serialized_attributes can migrate to Rails 5? I'm sure sound reasons exist to deprecate this feature, but breaking API changes without (what appears to be) a viable path for migration worries me heavily.

Many Rails apps rely on popular gems that use serialized_attributes, such as paper_trail, rails_admin & identity_cache. Making sure these popular gems can gracefully migrate away from serialized_attributes is imperative IMO, or else I see existing projects facing a wall of frustration when they try to upgrade.

@senny
Copy link
Member

senny commented Mar 4, 2015

@KelseyDH I feel like we have discussed this matter over and over again. The mentioned gems have already taken action to prevent the warnings and be ready for Rails 5.

Help was provided in these discussions and a viable solution was found.

While Rails does not offer a direct successor to serialized_attribtues, there are ways to achieve the same behavior when necessary. The new type-casting facility also makes some uses of this method obsolete.

Let us know when there are concrete issues with upgrading on a gem or application and we'll be happy to discuss it.

@KelseyDH
Copy link

KelseyDH commented Mar 4, 2015

@senny thank you for responding. I chimed in because a viable upgrade path did not appear to exist within these discussions. I'm happy to see good progress is under way.

You might be receiving more attention than expected for this deprecation because of people mistakenly associating a deprecation of serialized_attributes with a deprecation of serialize. (I admit I made that mistake until I looked closer at the PR you provided.)

@jlaw90
Copy link

jlaw90 commented Mar 14, 2015

I am currently working on a project that includes per-attribute searching. As a sensible default for which columns to search, it grabs all columns with content_columns and then removes all serialized attributes using serialized_attributes - seeing as searching serialized attributes is probably not what's intended.

I don't see how this particular use case could be safely upgraded by using polymorphism?

@mathieujobin
Copy link

mathieujobin commented Oct 10, 2016

Why isn't there anything on the upgrade guide about this?

http://guides.rubyonrails.org/upgrading_ruby_on_rails.html

bootleq added a commit to bootleq/migrant that referenced this pull request Oct 15, 2016
Rails 4.2 start to deprecate `serialized_attributes`
rails/rails#15704

We can use alternative way (try AR column#cast_type) similar to what
globalize gem did:
https://github.com/globalize/globalize/pull/402/files
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

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