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

Refactored the methods into a HashPassword class #28

Closed
wants to merge 1 commit into from

Conversation

maxxon15
Copy link

Refactored the methods into a HashPassword class for better code
organization when using OOP in projects.

$pwObj = new HashPassword();
$hashedPasswd = $pwObj->password_hash($passwd, PASSWORD_BCRYPT, ["cost" => 10]);
echo $hashedPasswd;

Refactored the methods into a HashPassword class for better code
organization when using OOP in projects.
@phindmarsh
Copy link

From the readme:

This library is intended to provide forward compatibility with the password_* functions being worked on for PHP 5.5.

Your change will break this forward compatibility. This refactor is better suited to a forked library only.

@ircmaxell
Copy link
Owner

Before you submit a change like this, it would be worth trying to understand why it was designed like this in the first place. I would suggest giving this blogpost on the subject a read...

Thanks anyway!

@ircmaxell ircmaxell closed this May 28, 2013
@maxxon15
Copy link
Author

@ircmaxell Didn't know you had considered those scenarios. Maybe you could have included the link to your blog in the Readme?
and @phindmarsh is right too. I overlooked the fact that the APIs and algos may change and my code might break.

From the blog: "You could say that it organizes it better. But in reality, does it? In most applications, you're only going to be using this in at most one class. "

Well, I'm using it twice so, making a class probably makes sense in my case.

@staabm
Copy link
Contributor

staabm commented May 28, 2013

The goal is an in-place replacement for the PHP api to generate passwords, see http://de2.php.net/manual/en/ref.password.php. You will loose forward compatibilty after reorganizing it into a class.

You should use the php native api like it is (which exists since PHP5.5) and include this lib so you don't need to bother for PHP versions < 5.5.

@maxxon15
Copy link
Author

@staabm I'm using PHP 5.4.7

@staabm
Copy link
Contributor

staabm commented May 28, 2013

so you need to include this lib and use the API as described on http://de2.php.net/manual/en/ref.password.php.

nothing more todo

@maxxon15
Copy link
Author

@staabm I don't wanna repeat myself every time I need those functions. You know... following the DRY principles and stuff. That's why I tried organizing them into a class.

If this is gonna break if I put it into a class, should I just go ahead and make my own class? And then use these functions from there?

@bazo
Copy link

bazo commented May 28, 2013

You have functions, so it is alredy DRY...

@staabm
Copy link
Contributor

staabm commented May 28, 2013

After you organized it into classes you cannot use it as functions anymore, because they get methods.

In case you like to abstract that you are using the PHP native api, you may add a layer which calls the native functions, but reorganize this lib into classes/methods doesnt make sense.

@maxxon15
Copy link
Author

Great! I guess I'll have to do just that.

Thanks for helping out! :-)

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

5 participants