Skip to content
This repository has been archived by the owner on Jun 16, 2024. It is now read-only.

Various cleanup #27

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Various cleanup #27

wants to merge 3 commits into from

Conversation

bakura10
Copy link
Contributor

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.

  1. 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
Copy link

Coverage Status

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

$bcrypt = new Bcrypt();

if ($serviceLocator instanceof AbstractPluginManager) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this check because this is not needed. "bcrypt" is inside a plugin manager so the service locator will always be a plugin manager. People that want to add a new BcryptHandler will add it in the handler plugin manager so this is no problem.

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@DASPRiD
Copy link
Member

DASPRiD commented Aug 30, 2013

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.

@@ -28,14 +28,9 @@ class BcryptFactory implements FactoryInterface
*/
public function createService(ServiceLocatorInterface $serviceLocator)
{
$config = $serviceLocator->getServiceLocator()->get('BaconUser\Config');
Copy link

Choose a reason for hiding this comment

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

This produces an error for me in PHPUnit:
Fatal error: Call to undefined method Zend\ServiceManager\ServiceManager::getServiceLocator() in /Password/Factory/BcryptFactory.php on line 31

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$serviceLocator is not a plugin manager then... It should! So lileky the object that ask this bcrypt factory uses the global service manager instead of using the proper plugin manageR.

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