Bacon / BaconUser

BaconUser provides simple user management
BSD 2-Clause "Simplified" License
14 stars 9 forks source link

Various cleanup #27

Open bakura10 opened 11 years ago

bakura10 commented 11 years ago

There are a various other things I've seen but I'd like to discuss with @Ocramius and @DASPRID before changing it:

1) There are a lot of factories scattered at different places in the code. For instance, "Password" have a subfolder "Factory", while Repositories factories are in the top-root "Factory" folder. I'd like if we had somehting cohernet on that. As the module is pretty specific and should not end with a lot of new features everyday, I'd be in favor of creating a Factory folder inside "Repository" too as well in "Options". The only one that makes sense to stay at top level is "ConfigFactory".

2) There is a "allowed_login_states" option, but I can't understand it. It's not used across the code base, adn the name is a bit misleading. I think it should be called "allowed_user_states" and be used either in the User entity before setting the user state, or somewhere in a service, throwing some kind of "InvalidUserStateException". But even, I don't really like this method. I think the user state should belong to the entity class and be defined there. There are not really "options", they are part of the entity's logic. So maybe we could remove it and doing something like that in the User entity:

protected $allowedStates = null;

public function setState($state)
{
    if (!is_array($this->allowedStates)  || !in_array($state, $this->allowedStates) {
        throw Exception;
    }

    $this->state = $state;
}

And people shoudl just extend the User class, and setting the allowedStates property.

3) For all the Options classes, it makes no sense to have fluent interfaces, as they are created and used by factories. I'd be in favour of removing all the return $this for all options classes.

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling 12c787cb0bb15a2dfe5db2ece0c32fc3586542f4 on bakura10:various-cleanup into 82f48cf0e086f499dc1eba00229d922f0ad8b4da on Bacon:master.

coveralls commented 11 years ago

Coverage Status

Coverage decreased (-0.0%) when pulling 0ec03cce1cc882fabd1289ed7904072a81ddb434 on bakura10:various-cleanup into 82f48cf0e086f499dc1eba00229d922f0ad8b4da on Bacon:master.

coveralls commented 11 years ago

Coverage Status

Coverage decreased (-0.0%) when pulling 808c7942a72c0d6955078121365ed89add0033ae on bakura10:various-cleanup into 82f48cf0e086f499dc1eba00229d922f0ad8b4da on Bacon:master.

DASPRiD commented 11 years ago

The allowed_login_states will be used in the authentication plugin. Only users with a state defined by that option will be allowed to log in.