Confirmable API

[nviennot]

Hi @josevalim,

As I was looking at the confirm! method in confirmable: https://github.com/plataformatec/devise/blob/85238014593443fb66cd0b1387f7ec8103f0fb91/lib/devise/models/confirmable.rb#L59-L84

And find the API a bit odd.

confirm! expects users to check for its return value, true or false, to detect possible errors.
This might not be a reasonable assumption. The name of the method implies that it would raise, similarly to the save vs save! behavior.

Here's some data to backup my claims:

• GitLab, a rails app made by fairly advanced coders exhibits the mistake: https://github.com/gitlabhq/gitlabhq/blob/0991d19c0d157025db8e9bf01d216771a4dbfe31/app/controllers/admin/users_controller.rb#L76
• Lots of small applications contain this mistake: https://github.com/search?l=ruby&o=desc&p=1&q=%22user.confirm%21%22+filename%3Acontroller+NOT+test+NOT+Factory+NOT+colchonet&ref=searchresults&s=indexed&type=Code&utf8=%E2%9C%93

So I'd say that confirm! should use save! instead of returning true/false. Or be renamed to just confirm?, or try_confirm, to make it explicit to the user should check for the return value.

What do you think?

Side note: I have personal interest to go with the save! route, because NoBrainer's save! method has the same semantics as other ORMs, unlike save which behaves like save!.

[josevalim]

Thanks. I don't think we use save! anywhere though, do we? The issue is that using save! would extend Devise's API... So we should either not use save! and then raise ourselves or rename the method to confirm (keep confirm! with a deprecation warning).

[nviennot]

save! is not used in the codebase, only save and save(validate: false).

There is only one other method where validations are run and the method is a bang: https://github.com/plataformatec/devise/blob/master/lib/devise/models/recoverable.rb#L42

so we might rename reset_password! to reset_password as well.

[josevalim]

Agreed, that would be best!​ Thanks for looking into this!

[josevalim]

Btw, can you please submit a pull request? :)

[nviennot]

I've submitted a PR (#3570).

---

Also I have a bit of a problem with compatiblity with NoBrainer and the adapter devise-nobrainer

The AR equivalent in NoBrainer of save is save? (and save! is save).
Everywhere save(validate: false) is used, it doesn't matter. But when validations are performed it matters as NoBrainer raises an exception while devise expect no exception.

There are two places save is called and performs validations:

• https://github.com/plataformatec/devise/blob/master/app/controllers/devise/registrations_controller.rb#L17
• https://github.com/plataformatec/devise/blob/master/lib/devise/models/confirmable.rb#L75-L77

Side node: there are 13 places where save is called without performing validations (that seems a bit odd, but whatever).

So to make NoBrainer compatible with devise, I make these two save calls to call NoBrainer's save?.
A few solutions:

1. Since it's only two call to change, it's simple enough to do a defined(:save?) ? save? : save.
2. We could modify orm_adapter to provide a layer of indirection for save
3. But the orm_adapter project doesn't seem to be much maintained. Further, orm_adapter is not really used by any other project besides devise. Popular libraries seem to provide in-library adapters, which is easier to maintain. (e.g. https://github.com/amatsuda/kaminari/tree/master/lib/kaminari/models). So we could remove the dependency on orm_adapter, and roll in-library adapters.

What do you think we should do?

[nviennot]

Before closing, can you address my latest comment?
Should I do a PR to go with 1) ?

[josevalim]

I could swear I had written a reply, sorry.

I don't think we should change Devise due to an ORM. The only short-term option I have in mind right now is to pass :validate option to all save calls, then you can monkey-patch no brainer and do the proper thing based on the :validate option.

The long-term option is to improve orm_adapter (I am a maintainer) to have a save function so we could do model.to_adapter.save(model) or something similar.

[nviennot]

going with the save(:validates => false) route will not work due to the uniqueness validations. (e.g. NoBrainer acquires locks to avoid races with uniqueness validations and persistence).

So let's do the orm_adapter solution :)
Should you do it, or should I make a PR (which I will do it next week though)?

[josevalim]

Please do send a PR!

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Lead Developer

[nviennot]

Done:

ianwhite/orm_adapter#45
#3587

[nviennot]

ping :)

[nviennot]

Nevermind, I no longer need the save abstraction.