From 1db9ddb0e6d63b750b4b4860aa2a3ccc6879eb8d Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Fri, 19 Feb 2016 10:23:38 -0600 Subject: [PATCH 1/6] Ensure navigation plugin manager initializer works properly - Adds a test to ensure that the parent container is injected into navigation helpers by the initializer created in the navigation PluginManager constructor, and fixes the code to work correctly. --- src/Helper/Navigation/PluginManager.php | 22 +++++++++++++++++- .../PluginManagerCompatibilityTest.php | 23 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/Helper/Navigation/PluginManager.php b/src/Helper/Navigation/PluginManager.php index 4f0d3490..d2c583fc 100644 --- a/src/Helper/Navigation/PluginManager.php +++ b/src/Helper/Navigation/PluginManager.php @@ -65,11 +65,31 @@ class PluginManager extends HelperPluginManager */ public function __construct($configOrContainerInstance = null, array $v3config = []) { - $this->initializers[] = function ($container, $instance) { + $this->initializers[] = function ($first, $second) { + // v2 vs v3 argument order + if ($first instanceof ContainerInterface) { + // v3 + $container = $first; + $instance = $second; + } else { + // v2 + $container = $second; + $instance = $first; + } + if (! $instance instanceof AbstractHelper) { return; } + // This initializer was written with v2 functionality in mind; as such, + // we need to test and see if we're called in a v2 context, and, if so, + // set the service locator to the parent locator. + // + // Under v3, the parent locator is what is passed to the method already. + if (method_exists($container, 'getServiceLocator') && $container->getServiceLocator()) { + $container = $container->getServiceLocator(); + } + $instance->setServiceLocator($container); }; diff --git a/test/Helper/Navigation/PluginManagerCompatibilityTest.php b/test/Helper/Navigation/PluginManagerCompatibilityTest.php index 28454ab4..ca61da07 100644 --- a/test/Helper/Navigation/PluginManagerCompatibilityTest.php +++ b/test/Helper/Navigation/PluginManagerCompatibilityTest.php @@ -14,6 +14,7 @@ use Zend\ServiceManager\Test\CommonPluginManagerTrait; use Zend\View\Exception\InvalidHelperException; use Zend\View\Helper\Navigation\AbstractHelper; +use Zend\View\Helper\Navigation\Breadcrumbs; use Zend\View\Helper\Navigation\PluginManager; /** @@ -65,4 +66,26 @@ public function testConstructorAllowsConfigInstanceAsFirstArgumentUnderV2() $helpers = new PluginManager(new Config([])); $this->assertInstanceOf(PluginManager::class, $helpers); } + + public function testInjectsParentContainerIntoHelpersUnderV2() + { + $helpers = $this->getPluginManager(); + if (method_exists($helpers, 'configure')) { + $this->markTestSkipped('This test is specific to v2 functionality'); + } + + $config = new Config([ + 'navigation' => [ + 'default' => [], + ], + ]); + + $services = new ServiceManager(); + $config->configureServiceManager($services); + $helpers = new PluginManager($services); + + $helper = $helpers->get('breadcrumbs'); + $this->assertInstanceOf(Breadcrumbs::class, $helper); + $this->assertSame($services, $helper->getServiceLocator()); + } } From d93331b42ae049eec29dd51e98beed1acbe83202 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Fri, 19 Feb 2016 10:24:01 -0600 Subject: [PATCH 2/6] `s/Config/config/` for service name in test - for forwards compatiblity --- test/Helper/Navigation/AbstractTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Helper/Navigation/AbstractTest.php b/test/Helper/Navigation/AbstractTest.php index 00ea68b9..1639d386 100644 --- a/test/Helper/Navigation/AbstractTest.php +++ b/test/Helper/Navigation/AbstractTest.php @@ -126,7 +126,7 @@ protected function setUp() 'extra_config' => [ 'service_manager' => [ 'factories' => [ - 'Config' => function () use ($config) { + 'config' => function () use ($config) { return [ 'navigation' => [ 'default' => $config->get('nav_test1'), From a742fafb29b87d500668306aa42068e08b2b053b Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Fri, 19 Feb 2016 10:31:44 -0600 Subject: [PATCH 3/6] Ensure parent manager is injected under v3 - Modified the condition for pulling the parent locator to test against the v3 `configure` method instead; `getServiceLocator()` also exists in v3, but emits a deprecation notice. - Modified the test for the behavior to work under both versions. The expected behavior is the same, given the same configuration. --- src/Helper/Navigation/PluginManager.php | 2 +- test/Helper/Navigation/PluginManagerCompatibilityTest.php | 7 +------ 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/Helper/Navigation/PluginManager.php b/src/Helper/Navigation/PluginManager.php index d2c583fc..a6ab8910 100644 --- a/src/Helper/Navigation/PluginManager.php +++ b/src/Helper/Navigation/PluginManager.php @@ -86,7 +86,7 @@ public function __construct($configOrContainerInstance = null, array $v3config = // set the service locator to the parent locator. // // Under v3, the parent locator is what is passed to the method already. - if (method_exists($container, 'getServiceLocator') && $container->getServiceLocator()) { + if (! method_exists($container, 'configure') && $container->getServiceLocator()) { $container = $container->getServiceLocator(); } diff --git a/test/Helper/Navigation/PluginManagerCompatibilityTest.php b/test/Helper/Navigation/PluginManagerCompatibilityTest.php index ca61da07..5a854257 100644 --- a/test/Helper/Navigation/PluginManagerCompatibilityTest.php +++ b/test/Helper/Navigation/PluginManagerCompatibilityTest.php @@ -67,13 +67,8 @@ public function testConstructorAllowsConfigInstanceAsFirstArgumentUnderV2() $this->assertInstanceOf(PluginManager::class, $helpers); } - public function testInjectsParentContainerIntoHelpersUnderV2() + public function testInjectsParentContainerIntoHelpers() { - $helpers = $this->getPluginManager(); - if (method_exists($helpers, 'configure')) { - $this->markTestSkipped('This test is specific to v2 functionality'); - } - $config = new Config([ 'navigation' => [ 'default' => [], From 52916971c6cb58181ad46f0797aab2cf36d18fce Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Fri, 19 Feb 2016 14:10:54 -0600 Subject: [PATCH 4/6] Added CHANGELOG for #50 --- CHANGELOG.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d870853..d43f0230 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ All notable changes to this project will be documented in this file, in reverse chronological order by release. -## 2.6.3 - TBD +## 2.6.3 - 2016-02-19 ### Added @@ -18,7 +18,11 @@ All notable changes to this project will be documented in this file, in reverse ### Fixed -- Nothing. +- [#50](https://github.com/zendframework/zend-view/pull/50) fixes + the initializer defined and registered in + `Navigation\PluginManager::__construct()` to ensure it properly pulls and + injects the application container into navigation helpers, under both + zend-servicemanager v2 and v3. ## 2.6.2 - 2016-02-18 From 9058a156766397ba67e90947405fd77d9dcf859f Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Mon, 22 Feb 2016 08:30:40 -0600 Subject: [PATCH 5/6] Ensure that lazy-instantiated plugin manager gets parent service locator Adds code to the `getPluginManager()` implementation to ensure that if a service locator is composed, it is used to inject a lazy-instantiated navigation plugin manager. This should fix the last lingering issues as reported in #49. --- .travis.yml | 2 +- composer.json | 1 + src/Helper/Navigation.php | 2 +- test/Helper/Navigation/AbstractTest.php | 9 ++++-- test/Helper/Navigation/NavigationTest.php | 35 ++++++++++++++++++----- 5 files changed, 38 insertions(+), 11 deletions(-) diff --git a/.travis.yml b/.travis.yml index 35dab11f..205e6840 100644 --- a/.travis.yml +++ b/.travis.yml @@ -53,7 +53,7 @@ before_install: - if [[ $ZEND_EVENTMANAGER_VERSION == '' ]]; then composer require --no-update "zendframework/zend-eventmanager:^3.0" ; fi - if [[ $ZEND_SERVICEMANAGER_VERSION != '' ]]; then composer require --dev --no-update "zendframework/zend-servicemanager:$ZEND_SERVICEMANAGER_VERSION" ; fi - if [[ $ZEND_SERVICEMANAGER_VERSION == '' ]]; then composer require --dev --no-update "zendframework/zend-servicemanager:^3.0.3" ; fi - - if [[ $ZEND_SERVICEMANAGER_VERSION == '' ]]; then composer remove --dev --no-update zendframework/zend-mvc zendframework/zend-session ; fi + - if [[ $ZEND_SERVICEMANAGER_VERSION == '' ]]; then composer remove --dev --no-update zendframework/zend-modulemanager zendframework/zend-mvc zendframework/zend-session ; fi install: - travis_retry composer install --no-interaction --ignore-platform-reqs diff --git a/composer.json b/composer.json index 5a6a7b77..f8601d37 100644 --- a/composer.json +++ b/composer.json @@ -30,6 +30,7 @@ "zendframework/zend-i18n": "^2.6", "zendframework/zend-json": "^2.6.1", "zendframework/zend-log": "^2.7", + "zendframework/zend-modulemanager": "^2.5", "zendframework/zend-mvc": "^2.6.1", "zendframework/zend-navigation": "^2.5", "zendframework/zend-paginator": "^2.5", diff --git a/src/Helper/Navigation.php b/src/Helper/Navigation.php index 8891d0e2..8eb37b22 100644 --- a/src/Helper/Navigation.php +++ b/src/Helper/Navigation.php @@ -322,7 +322,7 @@ public function setPluginManager(Navigation\PluginManager $plugins) public function getPluginManager() { if (null === $this->plugins) { - $this->setPluginManager(new Navigation\PluginManager()); + $this->setPluginManager(new Navigation\PluginManager($this->getServiceLocator())); } return $this->plugins; diff --git a/test/Helper/Navigation/AbstractTest.php b/test/Helper/Navigation/AbstractTest.php index 1639d386..36ba4a1c 100644 --- a/test/Helper/Navigation/AbstractTest.php +++ b/test/Helper/Navigation/AbstractTest.php @@ -86,7 +86,7 @@ abstract class AbstractTest extends \PHPUnit_Framework_TestCase */ protected function setUp() { - if (! class_exists(PluginFlashMessenger::class)) { + if (! class_exists(ServiceManagerConfig::class)) { $this->markTestSkipped( 'Skipping zend-mvc-related tests until that component is updated ' . 'to be forwards-compatible with zend-eventmanager, zend-stdlib, ' @@ -139,7 +139,10 @@ protected function setUp() ], ]; - $sm = $this->serviceManager = new ServiceManager(new ServiceManagerConfig); + $sm = $this->serviceManager = new ServiceManager(); + $sm->setAllowOverride(true); + + (new ServiceManagerConfig())->configureServiceManager($sm); $sm->setService('ApplicationConfig', $smConfig); $sm->get('ModuleManager')->loadModules(); $sm->get('Application')->bootstrap(); @@ -148,6 +151,8 @@ protected function setUp() $sm->setService('nav1', $this->_nav1); $sm->setService('nav2', $this->_nav2); + $sm->setAllowOverride(false); + $app = $this->serviceManager->get('Application'); $app->getMvcEvent()->setRouteMatch(new RouteMatch([ 'controller' => 'post', diff --git a/test/Helper/Navigation/NavigationTest.php b/test/Helper/Navigation/NavigationTest.php index c5e9ffaf..a1e3cc83 100644 --- a/test/Helper/Navigation/NavigationTest.php +++ b/test/Helper/Navigation/NavigationTest.php @@ -9,7 +9,9 @@ namespace ZendTest\View\Helper\Navigation; +use Interop\Container\ContainerInterface; use Zend\Navigation\Navigation as Container; +use Zend\Navigation\Page; use Zend\Permissions\Acl; use Zend\Permissions\Acl\Role; use Zend\ServiceManager\ServiceManager; @@ -61,11 +63,10 @@ public function testAcceptAclShouldReturnGracefullyWithUnknownResource() $this->_helper->setRole($acl['role']); $accepted = $this->_helper->accept( - new \Zend\Navigation\Page\Uri([ + new Page\Uri([ 'resource' => 'unknownresource', 'privilege' => 'someprivilege' - ], - false) + ], false) ); $this->assertEquals($accepted, false); @@ -144,10 +145,9 @@ public function testServiceManagerIsUsedToRetrieveContainer() $serviceManager = new ServiceManager; $serviceManager->setService('navigation', $container); - $pluginManager = new View\HelperPluginManager; - $pluginManager->setServiceLocator($serviceManager); + $pluginManager = new View\HelperPluginManager($serviceManager); - $this->_helper->setServiceLocator($pluginManager); + $this->_helper->setServiceLocator($serviceManager); $this->_helper->setContainer('navigation'); $expected = $this->_helper->getContainer(); @@ -558,7 +558,7 @@ public function testMultipleNavigationsWithSameHelperAndSameContainer() public function testSetPluginManagerAndView() { - $pluginManager = new \Zend\View\Helper\Navigation\PluginManager(); + $pluginManager = new Navigation\PluginManager(); $view = new PhpRenderer(); $helper = new $this->_helperName; @@ -568,6 +568,27 @@ public function testSetPluginManagerAndView() $this->assertEquals($view, $pluginManager->getRenderer()); } + /** + * @group 49 + */ + public function testInjectsLazyInstantiatedPluginManagerWithCurrentServiceLocator() + { + $services = $this->prophesize(ContainerInterface::class)->reveal(); + $helper = new $this->_helperName; + $helper->setServiceLocator($services); + + $plugins = $helper->getPluginManager(); + $this->assertInstanceOf(Navigation\PluginManager::class, $plugins); + + if (method_exists($plugins, 'configure')) { + // v3 + $this->assertAttributeSame($services, 'creationContext', $plugins); + } else { + // v2 + $this->assertSame($services, $plugins->getServiceLocator()); + } + } + /** * Returns the contens of the expected $file, normalizes newlines * @param string $file From 28885913a92771f261251c70792a725cb9b52b46 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Mon, 22 Feb 2016 08:41:45 -0600 Subject: [PATCH 6/6] Ensure that we get the parent locator Some tests are injecting the plugin manager as the service locator, when, in fact, the two instances should be discretely different. This patch ensures the tests continue to pass by updating `setServiceLocator()` to test if the instance is a plugin manager. If it is, it pulls the parent context and uses that instead. --- src/Helper/Navigation/AbstractHelper.php | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/Helper/Navigation/AbstractHelper.php b/src/Helper/Navigation/AbstractHelper.php index 46c4e93c..88e1aded 100644 --- a/src/Helper/Navigation/AbstractHelper.php +++ b/src/Helper/Navigation/AbstractHelper.php @@ -11,6 +11,7 @@ use Interop\Container\ContainerInterface; use RecursiveIteratorIterator; +use ReflectionProperty; use Zend\EventManager\EventManager; use Zend\EventManager\EventManagerAwareInterface; use Zend\EventManager\EventManagerInterface; @@ -19,6 +20,7 @@ use Zend\Navigation; use Zend\Navigation\Page\AbstractPage; use Zend\Permissions\Acl; +use Zend\ServiceManager\AbstractPluginManager; use Zend\View; use Zend\View\Exception; @@ -753,6 +755,26 @@ public function hasRole() */ public function setServiceLocator(ContainerInterface $serviceLocator) { + // If we are provided a plugin manager, we should pull the parent + // context from it. + // @todo We should update tests and code to ensure that this situation + // doesn't happen in the future. + if ($serviceLocator instanceof AbstractPluginManager + && ! method_exists($serviceLocator, 'configure') + && $serviceLocator->getServiceLocator() + ) { + $serviceLocator = $serviceLocator->getServiceLocator(); + } + + // v3 variant; likely won't be needed. + if ($serviceLocator instanceof AbstractPluginManager + && method_exists($serviceLocator, 'configure') + ) { + $r = new ReflectionProperty($serviceLocator, 'creationContext'); + $r->setAccessible(true); + $serviceLocator = $r->getValue($serviceLocator); + } + $this->serviceLocator = $serviceLocator; return $this; }