Event system ZfcUser 3 seriously broken

roelvanduijnhoven commented 6 years ago

The way ZfcUser 3 works with events is seriously broken:

Not all objects register themselves correctly to the SharedEventManager. All services should implement EventManagerAwareInterface in order for Zend\Mvc to inject it propertly (plus: there is a bug in the EventProvider that comes bundled: https://github.com/ZF-Commons/ZfcUser/pull/677).
A lot of services attach and trigger events in their constructor. That has two problems:
- They are not usuable. See RegisterFilter. The only way to use it would normally be the shared manager. However that is in ZF3 only injected after construction using an initializer.
- They are overwritten. There is some business logic dependent on it in AdapterChainServiceFactory. If we were to properly mark the AdapterClass as a EventManagerAwareInterface capable service, the event manager would be overwritten directly after construction (again via the bundled Initializer of ZF3 in Mvc). And thus the adapters will never receive the authenticate event.
The way events are triggered in AdapterChain is super weird. And is different from how it worked under ZF2. The event is passed as the $target parameter! What we need to do is use the triggerEvent method of the EventManager.

I have a PR that fixes all of this. See #677

gkzsolt commented 6 years ago

Thank you, roelvanduijnhoven, your PR fixed one of our problems when we migrated an app to ZF3. We hooked to the 'authenticate' event of the ZfcUser AdapterChain, but our callback was never called. Now this works. We still have another problem related.

We attached to the 'init' event of the ZfcUser\Form\LoginFilter, in order to reset the validator chain (we don't need password length >= 6). This was done in our app's onBootstrap($e) function with this code:

$events = $e->getApplication()->getEventManager()->getSharedManager();
$events->attach('ZfcUser\Form\LoginFilter','init', function($e)  {
            $form = $e->getTarget();
            // we reset password validation, to avoid 6 characters limit
            $form->get("credential")->setValidatorChain( new \Zend\Validator\ValidatorChain);
        });

, but unfortunately it never got called. Now, that you removed $this->getEventManager()->trigger('init', $this) from LoginFilter constructor, I wander how to do this. Can you please advice?

roelvanduijnhoven commented 6 years ago

@gkzsolt I used a delegator to do this.

class AdaptLoginFormDelegator implements DelegatorFactoryInterface
{
    public function __invoke(ContainerInterface $container, $requestedName, callable $callback, array $options = null)
    {
        $form = $callback();

        $form->getInputFilter()->remove('credential');

        return $form;
    }
}

@Danielss89 Any thought on this PR? Would be great to merge it!

gkzsolt commented 6 years ago

@roelvanduijnhoven, excellent idea, thank you !

I agree that this PR would be great to merge in. Any thoughts from the main developers?

BrunoSpy commented 6 years ago

Sadly this project seems abandoned.... Any news @Danielss89 ?

Danielss89 commented 6 years ago

Sorry, i haven't had the time to look into this yet. How many of you are using this patch already?

roelvanduijnhoven commented 6 years ago

@Danielss89 We have been running it on our production environment for some time; but I created this PR: so not the best reference material ;).

jroedel commented 6 years ago

It'd be awesome to get this working. I'm starting to feel left behind in ZF2, but I can't upgrade without this package. I wish I knew ZF3 event manager well enough to be able to look this over.

gkzsolt commented 6 years ago

@Danielss89 We are also using it in production, and if it's not merged, risk to not keep with the master. Is there a reason to not merge it?

kingharrison commented 6 years ago

Solid working ZfcUser is the main reason we are holding out upgrading to ZF3 from ZF2.

roelvanduijnhoven commented 6 years ago

What about merging this PR to a zf3 branch for the time being? And mark the default branch EOL. When we agree that zf3 branch is stable we can bump a new 4.0 release that is ZF3-only.

@kingharrison Did you test this patch already? If not: please do so we are sure we are not missing anything.

Also tests need to be revised on my PR. Anybody that wants to help with that?

jarrettj commented 6 years ago

👍

kingharrison commented 6 years ago

It looks to work for me.

On May 22, 2018 at 8:45:28 AM, Roel van Duijnhoven (notifications@github.com) wrote:

What about merging this PR to a zf3 branch for the time being? And mark the default branch EOL. When we agree that zf3 branch is stable we can bump a new 4.0 release that is ZF3-only.

@kingharrison https://github.com/kingharrison Did you test this patch already? If not: please do so we are sure we are not missing anything.

Also tests need to be revised on my PR. Anybody that wants to help with that?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ZF-Commons/ZfcUser/issues/678#issuecomment-390976423, or mute the thread https://github.com/notifications/unsubscribe-auth/AF1OcJ2u1u0HX9SI8hsHpBg9QcO3UUU_ks5t1AhmgaJpZM4Qre3o .

stijnhau commented 5 years ago

Maybe this can be merged in now?

ZF-Commons / ZfcUser

Event system ZfcUser 3 seriously broken #678