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

examples: fix (2022-03) #11927

Merged
merged 3 commits into from
Mar 26, 2022
Merged

Conversation

maiha
Copy link
Contributor

@maiha maiha commented Mar 25, 2022

Let's catch up examples before Crystal 1.4 😃

Best regards,

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:docs topic:stdlib labels Mar 25, 2022
@maiha
Copy link
Contributor Author

maiha commented Mar 25, 2022

Also, there is an error in crypto/bcrypt, but I don't know how to fix it, so I am leaving it alone.
https://github.com/crystal-lang/crystal/blob/master/src/crypto/bcrypt.cr#L69

require "crypto/bcrypt"
password = Crypto::Bcrypt.new "secret", "salt_of_16_chars"
Unhandled exception: Invalid salt size (Crypto::Bcrypt::Error)
  from crystal/src/crypto/bcrypt.cr:93:5 in 'initialize'
  from crystal/src/crypto/bcrypt.cr:91:3 in 'new'

@straight-shoota
Copy link
Member

straight-shoota commented Mar 25, 2022

The second argument needs to be a base64 encoded string that represents a byte slice with size 16.

For example:

Crypto::Bcrypt.new("secret", "AQIDBAUGBwgJCgsMDQ4PEA==") # => $2a$11$AQIDBAUGBwgJCgsMDQ4PE.jO8j7sDnyP0tyPC/HrVgzzeEvGdmSN2

@straight-shoota
Copy link
Member

On second thought... maybe we shouldn't provide an actually working salt value in the example. I'm sure there would be people who just copy the example code and never change the salt...
In any case, the documentation should also mention the base64 encoding. So I think we need to improve that, but it's out of scope for this PR. It's fine if you leave it at the last commit or revert that. We can merge it either way and improve the documentation in a separate PR.

@Blacksmoke16
Copy link
Member

On second thought... maybe we shouldn't provide an actually working salt value in the example.

I'd go as far as saying:

You probably want to use Crypto::Bcrypt.hash_secret instead.

given there is really no reason to supply your own salt. PHP's docs even say:

image

@maiha
Copy link
Contributor Author

maiha commented Mar 25, 2022

maybe we shouldn't provide an actually working salt value in the example

Agreed.

I want to correct only the ones that are definitely wrong, so I will leave the others as they are.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

Thanks!

@beta-ziliani beta-ziliani added this to the 1.4.0 milestone Mar 25, 2022
@straight-shoota straight-shoota merged commit 220114d into crystal-lang:master Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:docs topic:stdlib
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants