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

ActionPack (Rails) 7 support #123

Merged
merged 2 commits into from
Dec 16, 2021
Merged

Conversation

pilaf
Copy link
Contributor

@pilaf pilaf commented Dec 16, 2021

Rails 7 removed ActiveSupport::Dependencies.safe_constantize (commit), which Spyke depends on, essentially breaking Spyke on Rails 7.

This PR fixes it by detecting if the old method still exists, and reverting to the new ActiveSupport::Inflector.safe_constantize if not.

@balvig
Copy link
Owner

balvig commented Dec 16, 2021

Thanks a lot for submitting this @pilaf!

As the comment says, this bit was more or less stolen "as-is" from Rails.

I think maybe we can follow the recommendation in the PR and use the public API of string.safe_constantize instead.

I've pushed a commit, could you maybe give it a shot to see if it works for you?

(and I guess I need to get tests set up for multiple versions of Rails 😅 )

Thanks again for spotting this! 👌

@pilaf
Copy link
Contributor Author

pilaf commented Dec 16, 2021

Hi @balvig, thanks for the quick reply!

You're right, for some reason I thought String#safe_constantize was a new addition in replacement of the AS::Dependencies method, but looking closely it's been there since the Rails 3 days.

I ran the tests using your commit against versions 4, 5, 6 and 7 of ActiveSupport, and it seems to work on all of them.

For versions 4 and 5 of ActiveSupport one test failed, but it seemed completely unrelated to this issue.

@balvig
Copy link
Owner

balvig commented Dec 16, 2021

@pilaf awesome, thanks a lot! 👍

@balvig balvig merged commit d34d558 into balvig:main Dec 16, 2021
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.

None yet

2 participants