-
Notifications
You must be signed in to change notification settings - Fork 31
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
Allow enabling for only certain STI models #33
base: master
Are you sure you want to change the base?
Allow enabling for only certain STI models #33
Conversation
…rd(record) patch, which, unlike the other patches, is only used when store_base_sti_class is *true*: if ActiveRecord::Base.store_base_sti_class if options[:source_type] through_record.send("#{source_reflection.foreign_type}=", options[:source_type]) end end
For example: ActiveRecord::Base.store_sti_classes = ['Person'] All other models will use the default behavior of storing the STI base class.
Have only changed it for Rails 5.1 so far. I can update the other patches too if there is interest and if this is likely to get merged. |
more intuitive what it does and that it expects an array of models
The reason I wanted this feature, in case it wasn't clear, was because:
Storing the STI class for all models was causing it to deviate from the standard behavior for a lot of models that had no need to use something other than the default Rails behavior. One of the benefits for storing the base class is preventing duplication of the STI type in both records.
In my case when I was trying to do some
I maybe could have moved those associations to the base class, but they really only made sense in that subclass, so I'd prefer to keep them there. But in order to use those associations through the polymorphic association, I apparently needed that association to store the actual subclass so that it could correctly look up its associations and avoid this error... |
I think this PR is very interesting. I wonder if there is instead an approach where classes that the features provided by this gem can explicitly provide:
I haven't tested it, but I wonder if that's already feasible. Thoughts? |
Great idea, @bboe. If I'm understand you correctly, you would prefer to configure how the STI class is stored directly within each model, instead of some global configuration done in an initializer. I think that would be a much better API than the current one, and more consistent with how ActiveRecord's own API for setting an
as well as more in line with how other If we took that approach, though, I would suggest we use a more descriptive name. Perhaps you would enable these features by setting
(or if you'd rather, by setting Sounds feasible to me... though I doubt I will be able to help implement that change. I've been using this branch in production since January without problem, but now I discovered that In order to avoid fighting against standard Rails behavior of always storing the base class in the inheritance_column, and risk more things breaking without warning in the future, I'm looking into abandoning this plugin altogether (if I can get everything working)... it's usually more trouble than it's worth to fight against Rails conventions. |
@TylerRick I think I'm going to close this PR due to inactivity. Is it worth updating for the current version? |
For example:
All other models will use the default behavior of storing the STI base class.