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

allow zf3 #335

Merged
merged 31 commits into from
Aug 11, 2016
Merged

allow zf3 #335

merged 31 commits into from
Aug 11, 2016

Conversation

prolic
Copy link
Collaborator

@prolic prolic commented Jun 23, 2016

  • Allow Zend-MVC 3.0 in composer
  • Allow Zend-Servicemanager 3.0 in composer
  • Allow Zend\Eventmanager 3.0 in composer
  • Update to ZendDeveloperTools 1.1 in composer
  • Update to DoctrineModule 1.1 in composer
  • Update to DoctrineORMModule 0.11 in composer
  • drop support of PHP 5.4 & 5.5
  • allow PHP 7

some further notes:

  1. DoctrineORMModule does not support Zend-MVC 3.0, but this module still can support it, as the tests are showing. If you want to run this module with DoctrineORMModule, then you have to use Zend-MVC 2.7 for now.

  2. There are some BC breaks in the factories. They are working the with Zend-Servicemanager v2 and v3 but do not implement the MutableCreationOptionsInterface, f.e. Also the plugin managers get the main service locator injected into the constructor, instead of calling $pluginmanager->setServiceLocator($sm);
    This has to be noted in the changelog. You don't need to care about this, when you simply use this module as is. If you extended or aggregated the provided factories, you need to update your code accordingly.

@coveralls
Copy link

coveralls commented Jun 23, 2016

Coverage Status

Coverage decreased (-0.2%) to 92.982% when pulling c359ffe on prolic:zf3 into 4937bc4 on ZF-Commons:master.

@danizord danizord self-assigned this Jun 24, 2016
@danizord
Copy link
Member

Mhmm seems like it is time to drop PHP 5.4 support?

@prolic
Copy link
Collaborator Author

prolic commented Jun 24, 2016

Even time to drop 5.5 I would say :-)

@danizord
Copy link
Member

danizord commented Jun 24, 2016

ok, I'm just affraid that our build does not run tests with both ZF2 and ZF3. I think you should bump dev dependency to latest version. Or should we drop ZF2 as well? :D

@prolic
Copy link
Collaborator Author

prolic commented Jun 24, 2016

Oh, no problem, let me do this... wait...

@prolic
Copy link
Collaborator Author

prolic commented Jun 24, 2016

so, that should do the trick.

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Coverage increased (+1.7%) to 94.84% when pulling 673874f on prolic:zf3 into 4937bc4 on ZF-Commons:master.

@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Coverage decreased (-0.1%) to 93.027% when pulling aefc37f on prolic:zf3 into 4937bc4 on ZF-Commons:master.

@prolic
Copy link
Collaborator Author

prolic commented Jun 24, 2016

damn. some tests are failing with minimum requirements already. I'll try to fix that tomorrow.

@prolic
Copy link
Collaborator Author

prolic commented Jun 24, 2016

Any ideas why latest zend-servicemanager installed is 2.7.6 instead of 3.1? Is there any other dependency requiring the older version perhaps?

@prolic
Copy link
Collaborator Author

prolic commented Jun 24, 2016

Damn, currently DoctrineModule and zend-developer-tools do not allow Zend-Servicemanager v3

for DoctrineModule, there is a PR here: doctrine/DoctrineModule#558
for ZendDeveloperTools there is only an issue here: zendframework/zend-developer-tools#207

@basz
Copy link
Collaborator

basz commented Jun 24, 2016

So you can prepare against that PR... I did that when I wanted to zf3 proof SlmQueue. Whenever doctrine orm is updated I'm fairly sure it will work...

Op 24 jun. 2016 om 16:02 heeft Sascha-Oliver Prolic notifications@github.com het volgende geschreven:

Damn, currently DoctrineModule and zend-developer-tools do not allow Zend-Servicemanager v3

for DoctrineModule, there is a PR here: doctrine/DoctrineModule#558
for ZendDeveloperTools there is only an issue here: zendframework/zend-developer-tools#207


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@prolic
Copy link
Collaborator Author

prolic commented Jul 8, 2016

Please review carefully. I needed to change a lot of code.

@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Coverage decreased (-1.3%) to 91.806% when pulling 1491665 on prolic:zf3 into 4937bc4 on ZF-Commons:master.

@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Coverage decreased (-1.3%) to 91.806% when pulling 52b2b23 on prolic:zf3 into 4937bc4 on ZF-Commons:master.

@prolic
Copy link
Collaborator Author

prolic commented Jul 20, 2016

the test should be failing with current implementation and pass with your
patch. it should work with zf2 and zf3, then we are safe.

Sascha-Oliver Prolic

2016-07-20 22:04 GMT+08:00 Atanas Vasilev notifications@github.com:

@nasko https://github.com/nasko Submit a PR to my zf3 branch and add
some unit tests (failing without your patch).

I'm not sure if I can do this properly. Isn't the purpose of the unit
tests to test implementation in isolation, while the issue I've encountered
is manifested when used in conjunction with zend-mvc and zend-eventmanager,
which both have been changed recently?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#335 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAYEvA1jk2W7wWFKxTQq0ephR1EipSaJks5qXirngaJpZM4I8koN
.

@nasko
Copy link

nasko commented Jul 21, 2016

I could really use some help here.

In my situation, if I change this:

    public function onResult(MvcEvent $event)
    {
        if ($this->isGranted($event)) {
            return;
        }
        $event->setError(self::GUARD_UNAUTHORIZED);
        $event->setParam('exception', new Exception\UnauthorizedException(
            'You are not authorized to access this resource',
            403
        ));
        $event->stopPropagation(true);
        $application  = $event->getApplication();
        $eventManager = $application->getEventManager();
        $eventManager->trigger(MvcEvent::EVENT_DISPATCH_ERROR, $event);
    }

... to this:

    public function onResult(MvcEvent $event)
    {
        if ($this->isGranted($event)) {
            return;
        }

        $event->setError(self::GUARD_UNAUTHORIZED);
        $event->setParam('exception', new Exception\UnauthorizedException(
            'You are not authorized to access this resource',
            403
        ));

        $event->stopPropagation(true);
        $event->setName(MvcEvent::EVENT_DISPATCH_ERROR);

        $target  = $event->getTarget();
        $target->getEventManager()->triggerEvent($event);
    }

The fatal error is not emitted and the redirect strategy does its job properly.

What I don't know is, how I could represent this with unit tests. This is the test I'm looking at:

ControllerGuardTest::testProperlySetUnauthorizedAndTriggerEventOnUnauthorization()

    public function testProperlySetUnauthorizedAndTriggerEventOnUnauthorization()
    {
        $event      = new MvcEvent();
        $routeMatch = new RouteMatch([]);

        $application  = $this->getMock('Zend\Mvc\Application', [], [], '', false);
        $eventManager = $this->getMock('Zend\EventManager\EventManagerInterface');

        $application->expects($this->once())
                    ->method('getEventManager')
                    ->will($this->returnValue($eventManager));

        $eventManager->expects($this->once())
                     ->method('trigger')
                     ->with(MvcEvent::EVENT_DISPATCH_ERROR);

        $routeMatch->setParam('controller', 'MyController');
        $routeMatch->setParam('action', 'delete');

        $event->setRouteMatch($routeMatch);
        $event->setApplication($application);

        $identityProvider = $this->getMock('ZfcRbac\Identity\IdentityProviderInterface');
        $identityProvider->expects($this->any())
                         ->method('getIdentityRoles')
                         ->will($this->returnValue('member'));

        $roleProvider = new InMemoryRoleProvider([
            'member'
        ]);

        $roleService = new RoleService($identityProvider, $roleProvider, new RecursiveRoleIteratorStrategy());

        $routeGuard = new ControllerGuard($roleService, [[
            'controller' => 'MyController',
            'actions'    => 'edit',
            'roles'      => 'member'
        ]]);

        $routeGuard->onResult($event);

        $this->assertTrue($event->propagationIsStopped());
        $this->assertEquals(ControllerGuard::GUARD_UNAUTHORIZED, $event->getError());
        $this->assertInstanceOf('ZfcRbac\Exception\UnauthorizedException', $event->getParam('exception'));
    }

You see, the event propagation, error and exception param are set okay, but when $eventManager->trigger(MvcEvent::EVENT_DISPATCH_ERROR, $event); is called, the event is passed further as the target and there the registered listeners are notified with the wrong event type:

namespace Zend\EventManager;

class EventManager implements EventManagerInterface
{
    public function trigger($eventName, $target = null, $argv = [])
    {
        // Zend\EventManager\Event and not Zend\Mvc\MvcEvent 
        // which all listeners expect
        $event = clone $this->eventPrototype; 
        $event->setName($eventName);
        $event->setTarget($target);
        $event->setParams($argv);

        return $this->triggerListeners($event);
    }
}

I don't know how to test this in isolation as in my opinion it's an integration issue. Could someone advise as to how this could be handled?

@bupy7
Copy link

bupy7 commented Jul 22, 2016

When planned merges these changes?

@nasko
Copy link

nasko commented Jul 22, 2016

it should work with zf2 and zf3, then we are safe

@prolic, How should I implement the tests to pass both with zf2 and zf3, with respect to changed namespaces? For example in ZF3 the Router is in its own namespace Zend\Router vs. Zend\Mvc\Router which is used in the current tests.
The tests won't pass on my environment, unless I edit the imports that reference the old namespace Zend\Mvc\Router

@michalbundyra
Copy link
Contributor

michalbundyra commented Jul 22, 2016

@nasko
Copy link

nasko commented Jul 22, 2016

@webimpress Thank you! The example is valid, but the same approach is not used with the current tests in the zf3 branch.

The tests fail with my environment even without modifying the code.

…ce of the wrong type to be passed to registered listeners
@nasko
Copy link

nasko commented Jul 22, 2016

Okay, I'm not creating a PR, because of said differences regarding imports, but here's my commit that basically fixes the problem for me: nasko@9e97ce3.

I'd be grateful for any feedback.

@danizord
Copy link
Member

@nasko you can skip the test if class_exists returns false.

@danizord
Copy link
Member

I'm going do merge it tonight, @bakura10 can you review it as well?

@coveralls
Copy link

coveralls commented Jul 25, 2016

Coverage Status

Coverage decreased (-0.2%) to 92.9% when pulling 66466fc on prolic:zf3 into 4937bc4 on ZF-Commons:master.

@coveralls
Copy link

coveralls commented Jul 25, 2016

Coverage Status

Coverage decreased (-0.2%) to 92.9% when pulling c895f5f on prolic:zf3 into 4937bc4 on ZF-Commons:master.

@nasko
Copy link

nasko commented Jul 25, 2016

This is beyond me - I guess I'll have to resort to using my fork to address #340

@danizord danizord added this to the 2.6.0 milestone Jul 25, 2016
@prolic
Copy link
Collaborator Author

prolic commented Jul 25, 2016

updated, thanks to @nasko for helping to fix this.

@coveralls
Copy link

coveralls commented Jul 25, 2016

Coverage Status

Coverage decreased (-0.3%) to 92.793% when pulling f7e401a on prolic:zf3 into 4937bc4 on ZF-Commons:master.


/* @var \ZfcRbac\Service\AuthorizationService $authorizationService */
$authorizationService = $parentLocator->get('ZfcRbac\Service\AuthorizationService');
$authorizationService = $container->get('ZfcRbac\Service\AuthorizationService');

$routeGuard = new RoutePermissionsGuard($authorizationService, $this->options);
Copy link
Member

Choose a reason for hiding this comment

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

$options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed.

@coveralls
Copy link

coveralls commented Aug 11, 2016

Coverage Status

Coverage decreased (-0.3%) to 92.825% when pulling 4afa493 on prolic:zf3 into 4937bc4 on ZF-Commons:master.

@danizord danizord merged commit b47e9a2 into ZF-Commons:master Aug 11, 2016
@svycka
Copy link
Contributor

svycka commented Oct 6, 2016

@danizord @bakura10 can we have a release with this?

@prolic prolic deleted the zf3 branch October 6, 2016 09:28
@prolic prolic restored the zf3 branch October 13, 2016 05:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants