Deprecate add_hash and check_pass functions

[riverrun]

Problem

In version 4, the add_hash and check_pass functions were added. These functions were intended to be useful convenience functions that would reduce boilerplate code.

Unfortunately, it seems that several developers, especially new users of Comeonin, find these functions (and the resulting api) confusing. In addition, these functions do not seem to be widely used in authentication libraries, and, on reflection, they are probably not as useful as I had hoped.

Solution

I propose to deprecate these two functions in version 5.4, with the intention of removing them in version 6.0 (which will be in 2-3 months' time).

I hope that this will simplify the api and make comeonin, and the related password hashing libraries, easier to use.

Final note

This decision to remove these functions is not final. It also depends on the feedback I get in the next 2-3 months.

[josevalim]

I agree. :) We don't those in mix phx.gen.auth either!

[riverrun]

Deprecation notices (documentation) added to version 5.4.0

[JonRowe]

👋 What is the intended replacement? The deprecations do not mention what you should swap them out for? (As an aside I found this rather confusing because the warning comes from Bcrypt but there is no mention of this in their changelog, so prehaps amending the message to at least reference it is a comeonin deprecation)

[josevalim]

@JonRowe the easiest is to look at their current implementations. add_hash in particular is quite straight-forward: https://github.com/riverrun/comeonin/blob/master/lib/comeonin.ex#L32

[JonRowe]

When I eventually found the source of the deprecation I did just vendor the logic from check_pass, but this was a convient one liner it would be nice if something replaced the deprecated functions :)

[datafoo]

Coming late to the party but for me check_pass is useful and I do not understand why it was deprecated.

If there was still confusion related to riverrun/argon2_elixir#38, then improving documentation would probably have been better than simply removing useful functions.

Also, why is this issue still open considering the deprecation is effective.

[josevalim]

👍 for closing this issue.

[egeersoz]

Hi there. I started getting these messages after running mix deps.get and found myself here. I agree with datafoo and I do not understand why this issue was closed, considering:

1. That some users find these functions confusing should not be sufficient reason to remove them, since other users might (and in fact do) find them useful. For a widely used library like this, there needs to be a much higher bar for deprecations.
2. No proper replacement is recommended, either in the deprecation message or in the docs. In fact, the wiki still suggests using add_hash (and the linked example is a 404). If new users find these functions confusing, imagine how much more confused they will be when they follow the outdated wiki and get the deprecation errors. Vendoring the code from the library is not a solution IMHO.

[josevalim]

I submitted a PR to include the deprecation messages: #162 - I have also updated the wiki.