sonata-project / SonataUserBundle

Symfony SonataUserBundle
https://docs.sonata-project.org/projects/SonataUserBundle
MIT License
339 stars 488 forks source link

Bug in TwoStepVerificationCommand #1274

Closed eerison closed 3 years ago

eerison commented 3 years ago

After this #1272 pass, I generate a bug, sorry for this!

It's trying to inject null https://github.com/sonata-project/SonataUserBundle/blob/c4a3fb7bf51d047fff93eb3198bb6d1d91a88e53/src/Resources/config/command.xml#L6

as there isn't GoogleAuthenticator in my dependency https://github.com/sonata-project/SonataUserBundle/blob/c4a3fb7bf51d047fff93eb3198bb6d1d91a88e53/src/Resources/config/google_authenticator.xml#L4

this service is invalid https://github.com/sonata-project/SonataUserBundle/blob/c4a3fb7bf51d047fff93eb3198bb6d1d91a88e53/src/Resources/config/google_authenticator.xml#L7-L13

error

Argument 2 passed to Sonata\UserBundle\Command\TwoStepVerificationCommand::__construct() must be an instance of Sonata\User  
  Bundle\GoogleAuthenticator\Helper, null given, called in /usr/src/app/var/cache/dev/ContainerG2UYBkm/srcApp_KernelDevDebugC  
  ontainer.php on line 1438                                                                                                    

But as Helper is require, how can I resolve this :confused:

eerison commented 3 years ago

Maybe add Helper as nullable and check this parameter?

https://github.com/sonata-project/SonataUserBundle/blob/c4a3fb7bf51d047fff93eb3198bb6d1d91a88e53/src/Command/TwoStepVerificationCommand.php#L41

VincentLanglet commented 3 years ago

Now i understand the line

NEXT_MAJOR: make $helper and $userManager mandatory (but still nullable).

You should update the construct to

public function __construct(
        ?string $name,
        ?Helper $helper,
        ?UserManagerInterface $userManager
    )

And then you should keep the check at the beginning on the command

if (null === $this->helper) {
    throw new \RuntimeException('...');
}

if (null === $this->userManager) {
    throw new \RuntimeException('...');
}
eerison commented 3 years ago

@VincentLanglet do you think, this is necessary null === $this->userManage ? Because we have UserManager in user-bundle, then always it'll exist!

VincentLanglet commented 3 years ago

@VincentLanglet do you think, this is necessary null === $this->userManage ? Because we have UserManager in user-bundle, then always it'll exist!

We don't need to allow null as param in the construct then.