Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Refactored to use upcoming EventManager v3 #31

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/doc export-ignore
/test export-ignore
/vendor export-ignore
.coveralls.yml export-ignore
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
.*.sw*
.*.un~
nbproject
doc/html/
tmp/

clover.xml
Expand Down
9 changes: 7 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@

All notable changes to this project will be documented in this file, in reverse chronological order by release.

## 2.7.0 - TBD
## 3.0.0 - TBD

### Added

- Nothing.
- [#31](https://github.com/zendframework/zend-mvc/pull/31) adds three required
arguments to the `Zend\Mvc\Application` constructor: an EventManager
instance, a Request instance, and a Response instance.

### Deprecated

Expand All @@ -18,6 +20,9 @@ All notable changes to this project will be documented in this file, in reverse

### Fixed

- [#31](https://github.com/zendframework/zend-mvc/pull/31) updates the component
to use zend-eventmanager v3.

## 2.6.1 - TBD

### Added
Expand Down
11 changes: 6 additions & 5 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@
},
"require": {
"php": ">=5.5",
"zendframework/zend-eventmanager": "~2.5",
"zendframework/zend-eventmanager": "dev-develop as 2.7.0",
Copy link
Member

Choose a reason for hiding this comment

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

Note: should probably use zend-eventmanager dev-develop as 3.0.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't yet; not all components are updated to zend-eventmanager v3 yet, which means that there will be an unresolveable conflict. (I know; I tried. 😄)

"zendframework/zend-servicemanager": "~2.5",
"zendframework/zend-hydrator": "~1.0",
"zendframework/zend-form": "~2.6",
"zendframework/zend-stdlib": "~2.7"
"zendframework/zend-stdlib": "~2.7",
"container-interop/container-interop": "^1.1"
},
"require-dev": {
"zendframework/zend-authentication": "~2.5",
Expand All @@ -31,14 +32,14 @@
"zendframework/zend-inputfilter": "~2.5",
"zendframework/zend-json": "~2.5",
"zendframework/zend-log": "~2.5",
"zendframework/zend-modulemanager": "~2.6",
"zendframework/zend-modulemanager": "dev-develop as 2.7.0",
"zendframework/zend-session": "~2.5",
"zendframework/zend-serializer": "~2.5",
"zendframework/zend-text": "~2.5",
"zendframework/zend-uri": "~2.5",
"zendframework/zend-validator": "~2.5",
"zendframework/zend-version": "~2.5",
"zendframework/zend-view": "~2.5",
"zendframework/zend-view": "dev-develop as 2.6.0",
"fabpot/php-cs-fixer": "1.7.*",
"phpunit/PHPUnit": "~4.0"
},
Expand Down Expand Up @@ -67,7 +68,7 @@
"extra": {
"branch-alias": {
"dev-master": "2.6-dev",
"dev-develop": "2.7-dev"
"dev-develop": "3.0-dev"
}
},
"autoload-dev": {
Expand Down
69 changes: 69 additions & 0 deletions doc/book/migration.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
# Migration Guide

This is a guide for migration from version 2 to version 3 of zend-mvc.

## Application

The constructor signature of `Zend\Mvc\Application` has changed. Previously, it
was:

```php
__construct($configuration, ServiceManager $serviceManager)
```

and internally, it pulled the services `EventManager`, `Request`, and `Response`
from the provided `$serviceManager` during initialization.

The new constructor signature is:

```php
__construct(
$configuration,
ServiceManager $serviceManager,
EventManager $events,
RequestInterface $request,
ResponseInterface $response
)
```

making all dependencies explicit. The factory
`Zend\Mvc\Service\ApplicationFactory` was updated to follow the new signature.

This change should only affect users who are manually instantiating the
`Application` instance.

## EventManager initializer and ControllerManager event manager injection

zend-mvc provides two mechanisms for injecting event managers into
`EventManagerAware` objects. One is the "EventManagerAwareInitializer"
registered in `Zend\Mvc\Service\ServiceManagerConfig`, and the other is internal
logic in `Zend\Mvc\Controller\ControllerManager`. In both cases, the logic was
updated due to changes in the v3 version of zend-eventmanager.

Previously each would check if the instance's `getEventManager()` method
returned an event manager instance, and, if so, inject the shared event manager:

```php
$events = $instance->getEventManager();
if ($events instanceof EventManagerInterface) {
$events->setSharedManager($container->get('SharedEventManager'));
}
```

In zend-eventmanager v3, event manager's are now injected with the shared
manager at instantiation, and no setter exists for providing the shared manager.
As such, the above logic changed to:

```php
$events = $instance->getEventManager();
if (! $events || ! $events->getSharedManager()) {
$instance->setEventManager($container->get('EventManager'));
}
```

In other words, it re-injects with a new event manager instance if the instance
pulled does not have a shared manager composed.

This likely will not cause regressions in existing code, but may be something to
be aware of if you were previously depending on lazy-loaded event manager
state.
8 changes: 8 additions & 0 deletions doc/bookdown.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"title": "zend-mvc: MVC application provider",
"content": [
{"Intro": "../README.md"},
{"Migration Guide": "book/migration.md"}
],
"target": "./html"
}
52 changes: 33 additions & 19 deletions src/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Zend\EventManager\EventManagerAwareInterface;
use Zend\EventManager\EventManagerInterface;
use Zend\ServiceManager\ServiceManager;
use Zend\Stdlib\RequestInterface;
use Zend\Stdlib\ResponseInterface;

/**
Expand Down Expand Up @@ -103,15 +104,18 @@ class Application implements
* @param mixed $configuration
* @param ServiceManager $serviceManager
*/
public function __construct($configuration, ServiceManager $serviceManager)
{
public function __construct(
$configuration,
ServiceManager $serviceManager,
EventManagerInterface $events,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we provide defaults to avoid BC break?

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely agree here.

Copy link

Choose a reason for hiding this comment

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

This is a BC break but we are working to mvc 3.0 if it is a GOOD BC break this is a good time to merge it

Copy link
Member

Choose a reason for hiding this comment

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

@gianarb it is kind of a big one, tbh, and it can be avoided

Copy link
Member Author

Choose a reason for hiding this comment

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

Putting a note here as threads got broken up by me answering via email.

This change was for a reason, which I stated in the PR summary: I ended up with a circular reference issue with the ApplicationFactory + EventManagerAware initializer that resulted in invalid EventManager state. The result was that the EM instance injected no longer had the default listeners injected, which broke the entire thing.

As I noted in another comment elsewhere: hardly anybody directly instantiates the Application instance. The skeleton application pulls it from the service manager. As such, we can call this change out in the migration documentation for the very small percentage of users who are manually instantiating.

RequestInterface $request,
ResponseInterface $response
) {
$this->configuration = $configuration;
$this->serviceManager = $serviceManager;

$this->setEventManager($serviceManager->get('EventManager'));

$this->request = $serviceManager->get('Request');
$this->response = $serviceManager->get('Response');
$this->setEventManager($events);
$this->request = $request;
$this->response = $response;
}

/**
Expand Down Expand Up @@ -142,19 +146,20 @@ public function bootstrap(array $listeners = [])
$listeners = array_unique(array_merge($this->defaultListeners, $listeners));

foreach ($listeners as $listener) {
$events->attach($serviceManager->get($listener));
$serviceManager->get($listener)->attach($events);
}

// Setup MVC Event
$this->event = $event = new MvcEvent();
$event->setName(MvcEvent::EVENT_BOOTSTRAP);
$event->setTarget($this);
$event->setApplication($this)
->setRequest($this->request)
->setResponse($this->response)
->setRouter($serviceManager->get('Router'));
$event->setApplication($this);
$event->setRequest($this->request);
$event->setResponse($this->response);
$event->setRouter($serviceManager->get('Router'));

// Trigger bootstrap events
$events->trigger(MvcEvent::EVENT_BOOTSTRAP, $event);
$events->triggerEvent($event);
return $this;
}

Expand Down Expand Up @@ -294,13 +299,15 @@ public function run()
};

// Trigger route event
$result = $events->trigger(MvcEvent::EVENT_ROUTE, $event, $shortCircuit);
$event->setName(MvcEvent::EVENT_ROUTE);
$result = $events->triggerEventUntil($shortCircuit, $event);
if ($result->stopped()) {
$response = $result->last();
if ($response instanceof ResponseInterface) {
$event->setName(MvcEvent::EVENT_FINISH);
$event->setTarget($this);
$event->setResponse($response);
$events->trigger(MvcEvent::EVENT_FINISH, $event);
$events->triggerEvent($event);
$this->response = $response;
return $this;
}
Expand All @@ -311,14 +318,16 @@ public function run()
}

// Trigger dispatch event
$result = $events->trigger(MvcEvent::EVENT_DISPATCH, $event, $shortCircuit);
$event->setName(MvcEvent::EVENT_DISPATCH);
$result = $events->triggerEventUntil($shortCircuit, $event);

// Complete response
$response = $result->last();
if ($response instanceof ResponseInterface) {
$event->setName(MvcEvent::EVENT_FINISH);
$event->setTarget($this);
$event->setResponse($response);
$events->trigger(MvcEvent::EVENT_FINISH, $event);
$events->triggerEvent($event);
$this->response = $response;
return $this;
}
Expand Down Expand Up @@ -350,8 +359,13 @@ protected function completeRequest(MvcEvent $event)
{
$events = $this->events;
$event->setTarget($this);
$events->trigger(MvcEvent::EVENT_RENDER, $event);
$events->trigger(MvcEvent::EVENT_FINISH, $event);

$event->setName(MvcEvent::EVENT_RENDER);
$events->triggerEvent($event);

$event->setName(MvcEvent::EVENT_FINISH);
$events->triggerEvent($event);

return $this;
Copy link
Member

Choose a reason for hiding this comment

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

Newline before this line (keeps consistent with the changes above)

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted, and staged locally to push with other changes.

}
}
11 changes: 6 additions & 5 deletions src/Controller/AbstractController.php
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,14 @@ public function dispatch(Request $request, Response $response = null)
$this->response = $response;

$e = $this->getEvent();
Copy link
Member

Choose a reason for hiding this comment

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

While at it, maybe rename it $event? ;-)

Copy link

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Out of scope for this PR; I tried very hard only to update the code necessary to get the EM refactor working with it. If you feel strongly, open new PRs.

$e->setRequest($request)
->setResponse($response)
->setTarget($this);
$e->setName(MvcEvent::EVENT_DISPATCH);
$e->setRequest($request);
$e->setResponse($response);
$e->setTarget($this);

$result = $this->getEventManager()->trigger(MvcEvent::EVENT_DISPATCH, $e, function ($test) {
$result = $this->getEventManager()->triggerEventUntil(function ($test) {
return ($test instanceof Response);
});
}, $e);

if ($result->stopped()) {
return $result->last();
Expand Down
6 changes: 2 additions & 4 deletions src/Controller/ControllerManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
namespace Zend\Mvc\Controller;

use Zend\EventManager\EventManagerAwareInterface;
use Zend\EventManager\EventManagerInterface;
use Zend\EventManager\SharedEventManagerInterface;
use Zend\Mvc\Exception;
use Zend\ServiceManager\AbstractPluginManager;
use Zend\ServiceManager\ConfigInterface;
Expand Down Expand Up @@ -73,10 +73,8 @@ public function injectControllerDependencies($controller, ServiceLocatorInterfac
// is why the shared EM injection needs to happen; the conditional
// will always pass.
$events = $controller->getEventManager();
Copy link
Member

Choose a reason for hiding this comment

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

Will need to check this against an interface (as per comment below), as the aware interface defines no getter :-(

Copy link
Member Author

Choose a reason for hiding this comment

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

Please see my previous comment...
On Oct 1, 2015 6:38 AM, "Marco Pivetta" notifications@github.com wrote:

In src/Controller/ControllerManager.php
#31 (comment):

@@ -73,10 +74,10 @@ public function injectControllerDependencies($controller, ServiceLocatorInterfac
// is why the shared EM injection needs to happen; the conditional
// will always pass.
$events = $controller->getEventManager();

Will need to check this against an interface (as per comment below), as
the aware interface defines no getter :-(


Reply to this email directly or view it on GitHub
https://github.com/zendframework/zend-mvc/pull/31/files#r40903766.

Copy link
Member

Choose a reason for hiding this comment

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

👍

if (!$events instanceof EventManagerInterface) {
if (! $events || ! $events->getSharedManager() instanceof SharedEventManagerInterface) {
$controller->setEventManager($parentLocator->get('EventManager'));
} else {
$events->setSharedManager($parentLocator->get('SharedEventManager'));
}
}

Expand Down
6 changes: 3 additions & 3 deletions src/Controller/Plugin/Forward.php
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,9 @@ protected function detachProblemListeners(SharedEvents $sharedEvents)
$results[$id] = [];
foreach ($eventArray as $eventName => $classArray) {
$results[$id][$eventName] = [];
$events = $sharedEvents->getListeners($id, $eventName);
$events = $sharedEvents->getListeners([$id], $eventName);
foreach ($events as $currentEvent) {
$currentCallback = $currentEvent->getCallback();
$currentCallback = $currentEvent;

// If we have an array, grab the object
if (is_array($currentCallback)) {
Expand All @@ -193,7 +193,7 @@ protected function detachProblemListeners(SharedEvents $sharedEvents)

foreach ($classArray as $class) {
if ($currentCallback instanceof $class) {
$sharedEvents->detach($id, $currentEvent);
$sharedEvents->detach($currentEvent, $id);
Copy link
Member

Choose a reason for hiding this comment

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

This is a super-subtle bc break. Is it possible to revert this in the EVM instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. In the new version, the second argument is optional, the first required.
On Oct 1, 2015 6:25 AM, "Marco Pivetta" notifications@github.com wrote:

In src/Controller/Plugin/Forward.php
#31 (comment):

@@ -193,7 +193,7 @@ protected function detachProblemListeners(SharedEvents $sharedEvents)

                 foreach ($classArray as $class) {
                     if ($currentCallback instanceof $class) {
  •                        $sharedEvents->detach($id, $currentEvent);
    
  •                        $sharedEvents->detach($currentEvent, $id);
    

This is a super-subtle bc break. Is it possible to revert this in the EVM
instead?


Reply to this email directly or view it on GitHub
https://github.com/zendframework/zend-mvc/pull/31/files#r40902934.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Still very very very annoying BC break

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a rarely used feature, TBH; there are very few use cases currently for detaching listeners, much less shared listeners, and the Forward plugin here is literally the only place I've seen it in the ZF2 or Apigility code bases.

Though with React, I suspect it will becomes more common. Those users will adopt v3 anyways, as it's faster. 😄

$results[$id][$eventName][] = $currentEvent;
}
}
Expand Down
Loading