[HOLD] [FCM-8671] Rails 8 update#4
Conversation
Ruby 3 and rails 7 support
handle ruby 3.x keyword arguments delegation
Updates CI to support Ruby 3+ and Rails v7
Fix gem publish action
Updates push gem action with a valid ruby version
Write attribute with polymorphic integer type
Should still able to set other reflection successfully
Bump the version to 3.2.2
Upgrade rails to 7.2.x version; clean up unsupported rails and ruby version
* Update to allow for rails 8 compatibility * update gem version * modify rakefile to support both sqlite3 and mysql * modify the test matrix * cleanup Rakefile * Use sqlite3 for tests * update readme * set version to 3.4 * remove rails 6.x support * Test rails 7 with ruby 3.0
Rails 8.1 changed `ActiveRecord::PredicateBuilder::PolymorphicArrayValue#initialize`
from `(associated_table, values)` to `(reflection, values)`, dropping the
`@associated_table` ivar that this gem reads via
`@associated_table.send(:reflection)` in `type_to_ids_mapping`. Without an
update, every polymorphic-array predicate query (e.g. `Model.where(thing: [a, b])`)
raises `NoMethodError: undefined method 'reflection' for nil` on Rails 8.1.
The minimal fix: read `@reflection` directly when set (Rails 8.1+), fall back to
`@associated_table.send(:reflection)` for older Rails versions. The rest of the
method runs unchanged.
This also adds Gemfile.rails-8.1-stable to the CI matrix so future regressions
are caught upstream rather than in downstream apps.
## Verification
The existing spec at `polymorphic_integer_type_spec.rb:135` ("properly finds
the object when passing an array of sources") exercises exactly this code path:
Link.where(source: [source])
Verified locally:
- Rails 8.0: 41/41 passing
- Rails 8.1: 41/41 passing
- Rails 8.1 *without* this fix: 1 failure (the array-of-sources test) —
proves the test catches the regression and the fix resolves it.
Downstream apps (clio/accounting, clio/manage, clio/themis, etc.) currently
maintain in-app monkey patches for this; bumping to 3.5.0 lets them delete
those.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The existing array-of-sources test only exercises a single-type array (`Link.where(source: [cat])`). The fix's value also covers multi-type arrays (`Link.where(source: [cat, dog])`) — a broader branch of `type_to_ids_mapping` where the result hash spans multiple keys. Locks in coverage on both Rails 8.0 (backwards compat) and 8.1. Suggested by code review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Support Rails 8.1 in PolymorphicArrayValueExtension
gemspec for Rails 8 compatibility; add missing migration for places table
ihollander
left a comment
There was a problem hiding this comment.
Fun with forked dependencies 🙃
Instead of manually patching our fork, should we try to sync our fork with the upstream? It looks like there were a few commits related to Rails 8 compatibility on the upstream (clio#67, clio#68), might be nice to get our fork fully up-to-date as part of this work in case those other compatibility-related changes end up impacting us in ways that aren't covered by our tests...
git remote add upstream git@github.com:clio/polymorphic_integer_type.git
git fetch upstream
git merge upstream/master
I'm also wondering how big of a lift it would be to just stop using this gem altogether - as far as I can tell, we only use it in one place in the rma codebase (here), and it seems like this gem is a pretty common pain point for Rails upgrades... I can make a Jira card for that (separate from the Rails 8 upgrade) if that seems like a worthwhile direction to pursue.
…hic used in `rma`
e6bf083 to
073022e
Compare
…morphic_name guard + regression
|
Our fork is now updated with upstream, my actual changes are:
and the rest are upstream churn |
There was a problem hiding this comment.
Pull request overview
Updates this gem/fork for Rails 8.x compatibility while adding regression coverage for the fork-specific guard that preserves custom polymorphic_name behavior on plain polymorphic associations.
Changes:
- Add Rails 8.0/8.1 appraisal-style Gemfiles and expand CI matrix to cover Rails 8 with appropriate Ruby exclusions.
- Introduce new FCM regression fixtures/specs (
Country+legacy_source) and adjust test migrations/helpers accordingly. - Update gem metadata/docs/changelog to reflect Rails 8 compatibility and the fork delta.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/place.rb | Removes old Place fixture used for plain polymorphic testing. |
| spec/support/migrations/11_add_legacy_source_reference_to_links_table.rb | Removes legacy migration for legacy_source reference. |
| spec/support/migrations/10_create_country_table_and_legacy_source_reference.rb | Adds new fixture migration for countries + legacy_source columns. |
| spec/support/link.rb | Updates Link fixture to include plain legacy_source association and custom class resolution. |
| spec/support/country.rb | Adds Country fixture overriding polymorphic_name. |
| spec/spec_helper.rb | Simplifies migration execution and updates required fixtures. |
| spec/polymorphic_integer_type_spec.rb | Adds coverage for mixed-type arrays in where(source: [...]); removes older plain polymorphic test. |
| spec/fcm_polymorphic_name_guard_spec.rb | Adds regression specs asserting the fork guard behavior. |
| README.md | Updates setup guidance, DB compatibility notes, and documents the fork delta. |
| Rakefile | Switches DB rake tasks to SQLite-oriented behavior and normalizes formatting. |
| polymorphic_integer_type.gemspec | Updates dependency constraints and Ruby requirement; normalizes gemspec formatting. |
| lib/polymorphic_integer_type/version.rb | Bumps version to 3.5.0. |
| lib/polymorphic_integer_type/extensions.rb | Refactors belongs_to option handling and simplifies association extension hookup. |
| lib/polymorphic_integer_type/belongs_to_valid_options_extension.rb | Removes the (now unnecessary) ActiveRecord valid-options patch. |
| lib/polymorphic_integer_type/belongs_to_polymorphic_association_extension.rb | Reworks _type assignment behavior with the fork-specific guard for plain polymorphic associations. |
| lib/polymorphic_integer_type/activerecord_5_0_0/polymorphic_array_value_extension.rb | Updates Rails 8.1 compatibility for polymorphic array predicate building. |
| lib/polymorphic_integer_type.rb | Removes require for deleted valid-options extension. |
| gemfiles/Gemfile.rails-8.1-stable | Adds Rails 8.1 stable branch Gemfile. |
| gemfiles/Gemfile.rails-8.0-stable | Adds Rails 8.0 stable branch Gemfile. |
| gemfiles/Gemfile.rails-6.1-stable | Removes Rails 6.1 stable branch Gemfile. |
| CHANGELOG.md | Adds Rails 8.1 entry and appends Rails 8.0 notes. |
| .github/workflows/ci.yml | Adds Rails 8 gemfiles and expands Ruby matrix with version exclusions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ihollander
left a comment
There was a problem hiding this comment.
Re-upping approval, thanks for simplifying our patch and adding annotations around the FCM-specific functionality! It'll be nice to have these if we need to re-sync with the upstream again.
| owner[reflection.foreign_type] = | ||
| if owner.class.respond_to?("#{reflection.foreign_type}_mapping") | ||
| record.class.base_class | ||
| else | ||
| record.class.polymorphic_name | ||
| end |
There was a problem hiding this comment.
This is nice, much simpler than the multi-file change that we had before to fix the STI inheritance issue! Checking respond_to?("#{reflection.foreign_type}_mapping") seems like a good signal that the association is using this gem based on this line 👍
|
Also made a Jira card, if we want to consider removing our dependency on this gem or trying to get our patch merged in the upstream repo: https://radicle-health.atlassian.net/browse/FCM-8700 |
Overview / Approach
gemspecplacestableREADMETesting
This gem's own specs fully pass:
It's functionality within
rmawas fully tested in that repo.Database or Extract changes
N/A
Deployment plan
No deployment, this gem will be installed as part of
rmaRails 8 upgrade PR.HIPAA / Security
N/A
Billing
N/A