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

Rails 7 support #425

Closed

Conversation

movermeyer
Copy link

@movermeyer movermeyer commented Nov 11, 2022

What are you trying to accomplish?

Fixes #423.

Rails 7 introduced Active Record Encryption, which has names that collide with the names that attr_encrypted is using.

What approach did you choose and why?

I took over @ryosk7, @kineca, and @armiiller's closed PR, rebased it on latest master, and recreated it here.

It's a simple rename of class variables and methods, adding a attr_encrypted_ prefix to each.

IMO, it's a perfectly cromulent rename.

What should reviewers focus on?

This is a breaking change, since anyone relying on these names will have to update their code.

The impact of these changes

attr_encrypted will work once again on Rails 7

This was referenced Nov 11, 2022
Prefixes our encrypted_attributes with library specific attr_encrypted so we are not clashing with the Rails 7.0 definition of encrypted_attributes
@movermeyer movermeyer force-pushed the rails-7-0-support branch 4 times, most recently from b7310b2 to ee35c84 Compare November 11, 2022 16:40
Prefix encrypt and decrypt methods with attr_encrypted so we don't clash with rails 7
@xjunior
Copy link

xjunior commented Nov 15, 2022

Can you add rails 6 and 7 to the test matrix?

@xjunior
Copy link

xjunior commented Nov 15, 2022

I think we'll need a smarter matrix. Rails 7 isn't compatible with ruby <3.1.

@movermeyer
Copy link
Author

movermeyer commented Nov 15, 2022

I think we'll need a smarter matrix. Rails 7 isn't compatible with ruby <3.1.

@xjunior Also might be a good time to migrate to GitHub Actions?

@movermeyer
Copy link
Author

movermeyer commented Nov 15, 2022

Also, it seems that set_attribute_was was removed in Rails 6.

@jayakrishnang
Copy link

Hi @movermeyer, is there any workaround that you suggest we could try here?

@TheStranjer
Copy link

For what it's worth, this at least appears to work.

@jayakrishnang
Copy link

True, we could probably look at this patch to fix the set_attribute_was situation.
salsify#1

@mdh
Copy link

mdh commented Dec 9, 2022

I think we'll need a smarter matrix. Rails 7 isn't compatible with ruby <3.1.

From what I can see >= 2.7 is fine:
https://github.com/rails/rails/blob/v7.0.4/rails.gemspec#L12

@joshbranham
Copy link
Member

I was working with @mvastola but have been a bit busy. The idea was to cut a new major release for this breaking change and let rails 7 users upgrade. There is some discussion around how much we should maintain previous versions of this library for older Ruby and ActiveRecord versions. Fwiw we are using this exact patch on a fork at my company to get onto Rails 7, then figuring out how to migrate the data and move to native Rails encryption.

@joshbranham
Copy link
Member

True, we could probably look at this patch to fix the set_attribute_was situation. salsify#1

I am working to pull that in here #427

@guillaumebriday-pa
Copy link

I was working with @mvastola but have been a bit busy. The idea was to cut a new major release for this breaking change and let rails 7 users upgrade. There is some discussion around how much we should maintain previous versions of this library for older Ruby and ActiveRecord versions. Fwiw we are using this exact patch on a fork at my company to get onto Rails 7, then figuring out how to migrate the data and move to native Rails encryption.

Hi @joshbranham! First, thanks for your awesome work 👍

Do you have any idea when this could be released, to upgrade to Rails 7?

Thanks

@joshbranham
Copy link
Member

The current blocker is set_attribute_was doesn't work in >= 6 Activerecord. In removing it's support, some of the tests now fail per #431

I haven't had much luck understanding why those tests exist, and if anyone is using #attr_was style methods via attr_encrypted. I assume they are, but the way this all works feels brittle. So if you wanted to take a look at that, we could get tests passing and merge this and then you could at minimum use the commit off master.

vimalvnair added a commit to vimalvnair/attr_encrypted that referenced this pull request Mar 17, 2023
… with Rails 7

Prefix 'attr_encrypted' to encrypt and decrypt methods to avoid clash with Rails 7

Adopted from PR: attr-encrypted#425
mvastola pushed a commit that referenced this pull request Mar 28, 2023
… with Rails 7

Prefix 'attr_encrypted' to encrypt and decrypt methods to avoid clash with Rails 7

Adopted from PR: #425
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.

Undefined method #key? for nil:NilClass
8 participants