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

UW-2661: Add support for Ruby 3.x and Rails 7.x #16

Merged
merged 3 commits into from
Aug 30, 2022

Conversation

bfreese
Copy link
Contributor

@bfreese bfreese commented Aug 13, 2022

Adds support for ruby 3.x and Rails 7.x
Adds support for automatic publishing to github packages

@bfreese bfreese requested review from aemadrid and alauper August 13, 2022 20:20
@bfreese
Copy link
Contributor Author

bfreese commented Aug 13, 2022

@alauper I worked on this before I noticed you had started on #14

@bfreese bfreese self-assigned this Aug 14, 2022
Copy link

@alauper alauper left a comment

Choose a reason for hiding this comment

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

I'm testing MP against this branch.

One thing I'm noticing is previously unpermitted parameters worked with Actionable. But with these changes, the code tries to convert them to a hash first and then raise an exception saying

unable to convert unpermitted parameters to hash

That's a problem with the consumer (MP's bad code). But it does make this a break change - requiring code cleanup on the consumer side first.

Here's how I fixed one example
Screen Shot 2022-08-16 at 11 14 29 AM

.

@bfreese
Copy link
Contributor Author

bfreese commented Aug 17, 2022

I'm testing MP against this branch.

One thing I'm noticing is previously unpermitted parameters worked with Actionable. But with these changes, the code tries to convert them to a hash first and then raise an exception saying

@alauper Interesting. It would seem it is related to fixes in Strong Parameters in Rails 7. Are the changes required for rails 7 large in scope that we need to figure out a way to mitigate it? If so, do you have any suggestions we could apply here?

@alauper
Copy link

alauper commented Aug 18, 2022

@bfreese MP is already on Rails 7. I think it's related to how the parameters are being processed when using the **.

I brought this up to Hector to prioritize. I have a list of files that need to be updated and will chip away at it. If a higher priority doesn't get set, I should have MP updated within 2 weeks.

@bfreese
Copy link
Contributor Author

bfreese commented Aug 18, 2022

@bfreese MP is already on Rails 7. I think it's related to how the parameters are being processed when using the **.

I brought this up to Hector to prioritize. I have a list of files that need to be updated and will chip away at it. If a higher priority doesn't get set, I should have MP updated within 2 weeks.

Ok thanks for this information. We have not hit this yet as the project(s) that we have converted to Rails 7/Ruby 3 have already dealt with strong parameters.

@aemadrid Do you have any input on this PR?

@aemadrid
Copy link
Contributor

Don't have experience with Rails 7 and parameters. In this case the interface is that it expects params to be a hash or something that is safe to convert to a hash. Although I see that it would require a lot of work to convert the unsafe params into safe it does seem that it goes with the work of upgrading to Rails 7. All the solutions I can think of are hacks/ugly and only point out that the work of dealing with strong parameters is required. Otherwise it seems that everything else in the PR is kosher.

@bfreese
Copy link
Contributor Author

bfreese commented Aug 25, 2022

@alauper Any objection to merging this?

@alauper
Copy link

alauper commented Aug 25, 2022

@bfreese Yes, I think it's fine. MP should be ruby 3 compliant within a few more days.

@bfreese bfreese merged commit 526c309 into master Aug 30, 2022
@bfreese bfreese deleted the brianf/uw-2661/ruby_3_rails_7_support branch August 30, 2022 14:58
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