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

Delegated Type supports customizeable foreign_type column #45041

Merged
merged 1 commit into from
Mar 14, 2023

Conversation

jasonkarns
Copy link
Contributor

Adds support for customizing delegated_type's {role}_type column via the foreign_type option.

Summary

Delegated Types leans heavily on Rails’ existing polymorphic associations. The primary association created by delegated_type is a polymorphic belongs_to association.

For apps that are using polymorphic associations but need to customize the column containing the type/class, foreign_type is available as an option. It would be helpful if the same customization were available for Delegated Types.

Since delegated_types already passes its options along to the underlying belongs_to association, this is a relatively straightforward change. Instead of only deriving the type column by using "#{role}_type", it can respect a foreign_type option. (Same option key as used by polymorphic associations.)

Example:

delegated_type :entryable, types: %w[ Message Comment ], foreign_type: :entry_class

@skipkayhil
Copy link
Member

Can you add a CHANGELOG entry in Active Record? The change makes sense to me

@jasonkarns jasonkarns force-pushed the delegated-type-column branch 2 times, most recently from edda66e to d245e6d Compare November 2, 2022 16:46
@jasonkarns
Copy link
Contributor Author

@skipkayhil Added changelog entry and rebased. I assume the entry was supposed to go at the top. (I looked at its git-blame and most recent entries seemed to be at the top)

Happy to reword as desired.

@skipkayhil skipkayhil added the ready PRs ready to merge label Nov 2, 2022
@jasonkarns jasonkarns force-pushed the delegated-type-column branch 2 times, most recently from cbc5a26 to f68db84 Compare February 12, 2023 19:23
@jasonkarns
Copy link
Contributor Author

@skipkayhil rebased. any chance this can land in 7.1? Our team has an ugly monkey patch dealing with this right now.

@skipkayhil
Copy link
Member

any chance this can land in 7.1?

It just needs to be reviewed by a committer. I'll see if I can get it some attention in the Discord

@eileencodes
Copy link
Member

@jasonkarns can you add a test for this?

@skipkayhil skipkayhil removed the ready PRs ready to merge label Mar 1, 2023
@jasonkarns jasonkarns force-pushed the delegated-type-column branch 3 times, most recently from 5af9f51 to 23f8409 Compare March 12, 2023 23:30
@jasonkarns
Copy link
Contributor Author

@eileencodes tests added. I can squash/rebase again if necessary but wanted eyes on the tests before I do that. I'm not super familiar with the AR test suite opinions or patterns.

Copy link
Member

@eileencodes eileencodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple comments about documentation/changelog. Otherwise this looks good. Let me know when you've rebased and squashed your commits. The changelog entry goes at the top of the file when fixing the conflict.

activerecord/lib/active_record/delegated_type.rb Outdated Show resolved Hide resolved
activerecord/lib/active_record/delegated_type.rb Outdated Show resolved Hide resolved
activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
@jasonkarns
Copy link
Contributor Author

@eileencodes rebased!

@eileencodes eileencodes merged commit 95fe1f3 into rails:main Mar 14, 2023
@jasonkarns jasonkarns deleted the delegated-type-column branch March 14, 2023 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants