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

Using unsupported options causes a load error due to missing variable #41

Merged
merged 3 commits into from Jul 29, 2014
Merged

Using unsupported options causes a load error due to missing variable #41

merged 3 commits into from Jul 29, 2014

Conversation

johnathanludwig
Copy link
Contributor

Adding an option to attr_encrypted that is not supported by Symmetric Encryption causes an exception due to the params variable not being passed to the warn.

I've set the variable to be passed and added a test to confirm. Having the unsupported option present is all that is really needed to test as it prevents load, but I've added an explicit test so it will be clear why it is there in future changes.

I discovered this when using the gem tinfoil/devise-two-factor. This gem uses attr_encrypted's :key and :mode options here.

@reidmorrison
Copy link
Owner

Nice tests. I would prefer to just remove the params option altogether on the call to warn. This method was extracted from duplicated code for MySQL and Mongo. params is only available in the MySQL attr_encrypted call.

The other usage of params.inspect on the raise call can be changed to options.inspect.

@johnathanludwig
Copy link
Contributor Author

Alright, removed. I was thinking the same thing, 5 method parameters is a lot.

I didn't add options.inspect to the warn as it is already looping through options and warning on the leftovers individually.

reidmorrison added a commit that referenced this pull request Jul 29, 2014
Using unsupported options causes a load error due to missing variable
@reidmorrison reidmorrison merged commit 82a313b into reidmorrison:master Jul 29, 2014
@reidmorrison
Copy link
Owner

Thank you Johnathan. This issue would definitely hamper new adopters and the tests are most welcome.

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