Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

[BC break] Added the support of multiple users #4

Merged
merged 15 commits into from
Nov 27, 2017

Conversation

ezimuel
Copy link
Contributor

@ezimuel ezimuel commented Nov 20, 2017

This PR adds the support of multiple users, as suggested by #1. I updated the UserInterface changing the getUserRole() in getUserRoles(). Moreover, I added the following method to UserRepositoryInterface:

/**
 * Get the user roles if present
 *
 * @param string $username
 * @return array|null
 */
 public function getRolesFromUser(string $username): ?array;

This new function returns the roles of a user specified by $username. If the user has only one role, the result will be an array of one element. If the user does not have a role, the result will be null.
See the UserRepository\Htpasswd and UserRepository\PdoDatabase implementations.

@ezimuel ezimuel changed the title Added the support of multiple users [BC break] Added the support of multiple users Nov 20, 2017
@@ -18,7 +18,7 @@ public function getUsername(): string;
/**
* Get the user role
*
* @return string
* @return array|null

Choose a reason for hiding this comment

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

Why not @return string[]|null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks!

*/
public function getUserRole(): string;
public function getUserRoles(): ?array;

Choose a reason for hiding this comment

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

Why should this return null instead of an empty array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A null value indicates that the roles are not defined. An empty array can indicate that the roles are used but the user does not have a role.

Choose a reason for hiding this comment

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

Do you think this could make a difference for a consumer of the interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MidnightDesign not a big difference for the API but it matters from a semantic point of view, and we are also using a new feature of PHP 7.1.

Copy link
Member

Choose a reason for hiding this comment

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

I tend to agree with @MidnightDesign here. If the value type varies, you then need to add conditionals to your consumer:

$roles = $user->getUserRoles();
if (null === $roles) {
    // process differently
} else {
    foreach ($roles as $role) {
        // ...
    }
}

// or:
$roles = $user->getUserRoles() ?: [];
foreach ($roles as $role) {
    // ...
}

While the latter is succinct, it requires having the extra knowledge that a null value is possible, and an extra operation to ensure we have a list of roles once called. Compare this to always expecting an array:

foreach ($user->getUserRoles() as $role) {
     // ...
}

The above has the benefit of not requiring an intermediary variable prior to iteration. Additionally, if an empty array is returned, no iteration happens at all.

This was a design decision we approached with PSR-7 as well. I'd originally advocated for null returns for empty lists, but a number of folks showed me that consistency of return type has a ton of value.

[BC break] Added the support of multiple users

Conflicts:
	src/UserInterface.php
	src/UserRepository/PdoDatabase.php
	src/UserRepository/UserTrait.php
	src/UserRepositoryInterface.php
@weierophinney
Copy link
Member

@ezimuel I've rebased your branch and pushed the changes; unfortunately, I did it after a local merge, so there's a new merge commit.

I've also noted that I agree with changing the return value of getUserRoles() to simply array, and noting in the annotations @return string[].

@ezimuel
Copy link
Contributor Author

ezimuel commented Nov 27, 2017

@weierophinney ok for using a simple empty array as response, even if from a semantic point of view is a different output type.

@weierophinney weierophinney merged commit ef23c49 into zendframework:master Nov 27, 2017
weierophinney added a commit that referenced this pull request Nov 27, 2017
weierophinney added a commit that referenced this pull request Nov 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants