Skip to content

Add support for the rails 5.2 polymorphic name method#31

Merged
doliveirakn merged 2 commits intomasterfrom
polymorphic-names-support
Oct 28, 2020
Merged

Add support for the rails 5.2 polymorphic name method#31
doliveirakn merged 2 commits intomasterfrom
polymorphic-names-support

Conversation

@doliveirakn
Copy link
Copy Markdown
Contributor

@doliveirakn doliveirakn commented Oct 26, 2020

What problem does this solve

Prior to Rails 5.2, there wasn't a great consensus on what to use for the polymorphic_name of a class. In some instance it was the class name, in other instances, it seemed like it was the name that was used for single table inheritance.

As of Rails 5.2, there is now a canonical way which is to call polymorphic_name on the class. Now that there is a canonical way, we should remove the redundant lookups

How does this solve it

We don't want to completely break support, so this will check if a class responds to polymorphic_name and if so, it'll use that. It'll keep all of the other fallbacks in place though.

@doliveirakn doliveirakn force-pushed the polymorphic-names-support branch 3 times, most recently from 6f53b59 to 1d8f3ec Compare October 26, 2020 22:02
@doliveirakn doliveirakn force-pushed the polymorphic-names-support branch from 1d8f3ec to c1f717a Compare October 27, 2020 14:26
@Drew-Goddyn
Copy link
Copy Markdown
Contributor

Drew-Goddyn commented Oct 27, 2020

so this PR introduces a configuration variable that defaults to off

This doesn't seem to be the case anymore in favor just checking that it responds to polymorphic_name instead now?

@doliveirakn
Copy link
Copy Markdown
Contributor Author

@Drew-Goddyn that was the case the first attempt at support but is no longer the case. I'll remove that from the description.

Copy link
Copy Markdown

@daniel-almeida daniel-almeida left a comment

Choose a reason for hiding this comment

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

Left a clarifying question.

define_method "#{name}=" do |record|
super(record)
send("#{foreign_type}=", record.class)
association(name).loaded!
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What's the reason behind this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if we write the association in the super(record) and then change then *_type column, it will mark the association as stale, and lose the reference of it (causing it to want to lookup the object again).

By marking the assocaition as loaded afterwards, we guarantee that if once you write the record, it maintains that association.

@doliveirakn doliveirakn merged commit fa3a809 into master Oct 28, 2020
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.

3 participants