Rails 7 support - Githubissues

active-hash / active_hash

A readonly ActiveRecord-esque base class that lets you use a hash, a Yaml file or a custom file as the datasource

MIT License

1.19k stars 178 forks source link

Rails 7 support #272

Closed flavorjones closed 1 year ago

flavorjones commented 1 year ago

Alternative implementation to #267 that doesn't monkeypatch ActiveRecord.

This implementation subclasses ActiveRecord::Reflection::BelongsToReflection and overrides the compute_class method to allow ActiveHash associations.

I've also cherry-picked some relevant commits from #267.

kbrock commented 1 year ago

@flavorjones This looks great. A few thoughts:

Will this require developers to mixin this belongsto module everywhere for this functionality? (I vaguely remember this module in the past)
If so, we need to make sure this is a major version change with incompatibility note.
Do we need to document this change so it is easy for developers to fix when upgrading to 7.0?

(as with everywhere in rubyland. thanks for the contribution)

flavorjones commented 1 year ago

@kbrock Thanks for the review and for asking these questions.

This should "just work" for folks who have followed the README and are extending their AR classes with ActiveHash::Associations::ActiveRecordExtensions (see this section in the README).

However, it looks like not all the tests were following that advice but were still passing with Rails 6 (see the change in this PR to spec/active_hash/base_spec.rb), which seems suspicious and makes me believe there may be apps out there which are "working" but will break with Rails 7. Maybe I'm not fully understanding Active Hash and how it's supposed to be used? I could use some advice, honestly.

flavorjones commented 1 year ago

Looks like the ActiveRecordExtensions module was added in v0.9.6 (see 153c58ec) but wasn't added to this test (probably because it wasn't failing).

kbrock commented 1 year ago

Thanks. I thought I remembered this module from a number of years ago.

Ok, I change my view from breaking change to bug fix.

flavorjones commented 1 year ago

@kbrock I just saw now that I still had a "wip" message in that last commit, urk, sorry, I should have fixed that. Candidly, I did not expect that code to pass on every Rails version and assumed I'd be coming back to clean something up. :shrug:

kbrock commented 1 year ago

@flavorjones ugh. very sorry that I did not see that. ping me to get that through