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

Columns first and second params feels it is username / password #15

Closed
harikt opened this issue Jun 22, 2014 · 5 comments
Closed

Columns first and second params feels it is username / password #15

harikt opened this issue Jun 22, 2014 · 5 comments

Comments

@harikt
Copy link
Member

harikt commented Jun 22, 2014

The cols

$cols = array(
    'username',
    'password',
    'id',
    'email',
    'fullname',
    'website',
    'twitter'
);

$from = 'users';
$where = 'active = 1';

$pdoadapter = $auth_factory->newPdoAdapter(
    $dbh,
    PASSWORD_BCRYPT,
    $cols,
    $from,
    $where
);

assumes the array first and second value is username / password. That is a wrong assumption . May be we pass an associative array and don't use $cols[0] and $cols[1] in

protected function setCols($cols)
{
if (! isset($cols[0]) || trim($cols[0] == '')) {
throw new Exception\UsernameColumnNotSpecified;
}
if (! isset($cols[1]) || trim($cols[1] == '')) {
throw new Exception\PasswordColumnNotSpecified;
}
$this->cols = $cols;
}

@pmjones
Copy link
Member

pmjones commented Jun 22, 2014

It's not an assumption; the use passes the column names, so he says exactly what they are.

@harikt
Copy link
Member Author

harikt commented Jun 22, 2014

@pmjones not sure what you mean.

@pmjones
Copy link
Member

pmjones commented Jun 22, 2014

I mean, we're not assuming anything. The developer using the package specifies the columns he wants to select; we say the first one has to be the username column, and the second one has to be the password column. Am I missing your point maybe?

@harikt
Copy link
Member Author

harikt commented Jun 22, 2014

ok. $cols sounds the columns I need to fetch. So basically I just didn't made in the order they were.

So the order was something like

$cols = array(
    'username',
    'email',
    'id',
    'password',
    'fullname',
    'website',
    'twitter'
);

and I figured out that the username is what it expects to be column 0 and password as column 1. What I feel good is is if this is an associative array

$cols = array(
    'username' => 'username',
    'email',
    'id',
    'password' => 'password',
    'fullname',
    'website',
    'twitter'
);

the order doesn't matters. And we can throw really the exception that username is not present / password not present.

Else if you look at the first array both the cols[0] and cols[1] are present and we don't get an exception and only saying password incorrect exception. Which I feel is wrong. Docs may fix / mention first column is username and second column is password. But seems good to be associative in my opinion.

Thank you.

@pmjones
Copy link
Member

pmjones commented Jun 22, 2014

This does seem like a doc fix, not a technical issue.

Regarding associative, I was thinkging we want to leave the option for "column" => "alias" in the future.

@pmjones pmjones closed this as completed Jun 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants