Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

abstract factory for configs reading keys from merged config #4860

Merged
merged 13 commits into from

3 participants

Chris Raidler Matthew Weier O'Phinney Chris Raidler
Chris Raidler

this abstract factory can be activated in your service manager config like this:

'service_manager' => array(
    'abstract_factories' => array(
        'Zend\Config\AbstractConfigFactory'
    )
);

it will factor a \Zend\Config\Config object for requested names like

$serviceLocator->get('MyModule\Sub\Config');

where in this case the key MyModule\Sub has to exist in the merged Config.

Chris Raidler

travis failed on php 5.3.3 build only, all others are good. i cant find the issue here :-(

library/Zend/Config/AbstractConfigFactory.php
((7 lines not shown))
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace Zend\Config;
+
+use Zend\ServiceManager;
+
+/**
+ * Class AbstractConfigFactory
+ */
+class AbstractConfigFactory implements ServiceManager\AbstractFactoryInterface
+{
+ /**
+ * @var string
+ */
+ protected $pattern = '#^(.*)\\\\Config$#i';
Chris Raidler
sargech added a note

this could be done more performant using strrpos

Matthew Weier O'Phinney Owner

I'm honestly not sure about the regex.

I've seen a lot of modules not use their namespace for the module-specific config key, but instead a normalized version. As an example, ZfcUser uses the key "zfc-user"; I use "phly-blog", "phly-simple-page", etc. As such, using a namespace separator and class-like naming feels off.

It might make sense to have several regexes to check against, and allow injecting additional ones (developers could attach the abstract factory by instantiating it, injecting it, and then adding it to the ServiceManager). If so, I'd include the following:

  • #config[._-](.*)$#i
  • #^(.*)[\\\\._-]config$#i

That would likely capture the bulk of existing modules, allowing them to adopt the convention by either suffixing or prepending some verbiage ("config.", "config-", "config_", "-config", ".config", "_config", "\Config") to their existing keys based on the style they've already adopted.

Then, either add a setter or a constructor with an optional dependency that allows providing additional regex to use; these should be preferred over the defaults, so treat them like a stack when matching.

It will mean a little more work matching, but be a bit more flexible and allow for different naming styles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Matthew Weier O'Phinney

Please add unit tests, @sarge-ch

Chris Raidler

Thanks, added a TestCase, 3 Tests with 4 Assertionms. This ran though on travis:

[concat] ZendTest/Config
[concat]
[concat] PHPUnit 3.7.24 by Sebastian Bergmann.
[concat]
[concat] Configuration read from /home/travis/build/zendframework/zf2/tests/phpunit.xml.dist
[concat]
[concat] ............................................................... 63 / 140 ( 45%)
[concat] ..........................................SSSSSS............... 126 / 140 ( 90%)
[concat] ........SSSSSS
[concat]
[concat] Time: 2.16 seconds, Memory: 12.25Mb

Allthough it seems this branch has 2 failing Tests in ZendTest/View.

Matthew Weier O'Phinney

@sarge-ch I've fixed the failing tests on develop earlier today, which means I can review this without issue.

library/Zend/Config/AbstractConfigFactory.php
((23 lines not shown))
+
+ /**
+ * Determine if we can create a service with name
+ *
+ * @param ServiceManager\ServiceLocatorInterface $serviceLocator
+ * @param string $name
+ * @param string $requestedName
+ * @return bool
+ */
+ public function canCreateServiceWithName(ServiceManager\ServiceLocatorInterface $serviceLocator, $name, $requestedName)
+ {
+ if (!preg_match($this->pattern, $requestedName, $matches)) {
+ return false;
+ }
+
+ $config = $serviceLocator->get('Config');
Matthew Weier O'Phinney Owner

I'd do a check with has('Config') prior to this, and return false if that service is not available:

if (!$serviceLocator->has('Config')) {
    return false;
}
$config = $serviceLocator->get('Config');

That way no exception is thrown if the config service is unavailable, but the method will still correctly report it cannot create the service.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Chris Raidler

@weierophinney Many thanks for your feedback and thoughts. I've implemented your suggestions and wrote some tests accordingly.

library/Zend/Config/AbstractConfigFactory.php
((103 lines not shown))
+ foreach ($patterns as $pattern) {
+ $this->addPattern($pattern);
+ }
+
+ return $this;
+ }
+
+ /**
+ * @param array|\Traversable $patterns
+ * @return $this
+ * @throws \InvalidArgumentException
+ */
+ public function setPatterns($patterns)
+ {
+ if ($patterns instanceof \Traversable) {
+ $patterns = ArrayUtils::iteratorToArray($patterns);
Matthew Weier O'Phinney Owner

Since patterns are not a nested structure, you can just use iterator_to_array() here; I'll make that change on merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/AbstractConfigFactory.php
((107 lines not shown))
+ return $this;
+ }
+
+ /**
+ * @param array|\Traversable $patterns
+ * @return $this
+ * @throws \InvalidArgumentException
+ */
+ public function setPatterns($patterns)
+ {
+ if ($patterns instanceof \Traversable) {
+ $patterns = ArrayUtils::iteratorToArray($patterns);
+ }
+
+ if (!is_array($patterns)) {
+ throw new \InvalidArgumentException("patterns must be array or Traversable");
Matthew Weier O'Phinney Owner

Use the exceptions from the component. :)

(I'll make this change on merge)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/AbstractConfigFactory.php
((102 lines not shown))
+
+ foreach ($patterns as $pattern) {
+ $this->addPattern($pattern);
+ }
+
+ return $this;
+ }
+
+ /**
+ * @param array|\Traversable $patterns
+ * @return $this
+ * @throws \InvalidArgumentException
+ */
+ public function setPatterns($patterns)
+ {
+ if ($patterns instanceof \Traversable) {
Matthew Weier O'Phinney Owner

Import Traversable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/AbstractConfigFactory.php
((97 lines not shown))
+ }
+
+ if (!is_array($patterns)) {
+ throw new \InvalidArgumentException("patterns must be array or Traversable");
+ }
+
+ foreach ($patterns as $pattern) {
+ $this->addPattern($pattern);
+ }
+
+ return $this;
+ }
+
+ /**
+ * @param array|\Traversable $patterns
+ * @return $this
Matthew Weier O'Phinney Owner

s/$this/self/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/AbstractConfigFactory.php
((53 lines not shown))
+ return isset($config[$key]);
+ }
+
+ /**
+ * Create service with name
+ *
+ * @param ServiceManager\ServiceLocatorInterface $serviceLocator
+ * @param string $name
+ * @param string $requestedName
+ * @return mixed
+ */
+ public function createServiceWithName(ServiceManager\ServiceLocatorInterface $serviceLocator, $name, $requestedName)
+ {
+ $key = $this->match($requestedName);
+ $config = $serviceLocator->get('Config');
+ return new Config($config[$key]);
Matthew Weier O'Phinney Owner

Seeing as the Config service returns an array, I'd argue we should do the same here, to keep expectations the same.

Matthew Weier O'Phinney Owner

Also, since configuration shouldn't change between retrievals, cache the value of $config[$key], and return early if we already have it cached. I'd implement that something like:

if (isset($this->configs[$requestedName])) {
    return $this->configs[$requestedName];
}
$key = $this->match($requestedName);
if (isset($this->configs[$key])) {
    $this->configs[$requestedName] = $this->configs[$key];
    return $this->configs[$key];
}
$config = $serviceLocator->get('Config');
$config = $config[$key];
$this->configs[$requestedName] = $this->configs[$key] = $config;
return $config;

This will prevent the need to use PCRE on subsequent requests. I'd do similar logic for the canCreateServiceWithName() method as well, as a performance optimization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/Config/AbstractConfigFactory.php
((47 lines not shown))
+ $key = $this->match($requestedName);
+ if (null === $key) {
+ return false;
+ }
+
+ $config = $serviceLocator->get('Config');
+ return isset($config[$key]);
+ }
+
+ /**
+ * Create service with name
+ *
+ * @param ServiceManager\ServiceLocatorInterface $serviceLocator
+ * @param string $name
+ * @param string $requestedName
+ * @return mixed
Matthew Weier O'Phinney Owner

s/mixed/array/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Matthew Weier O'Phinney

@sarge-ch Looking good -- a bit more feedback to incorporate, and I think we may have a winner!

Chris Raidler

@weierophinney Thanks Matthew, added things :)

I could not really understand what you've meant with s/$this/self there ... however I'd be happy to change this as well as per your request. PHPStorm seems to only like $this a the @return - so I did not change it.

Matthew Weier O'Phinney

I could not really understand what you've meant with s/$this/self there

@return self is the accepted standard, and works across IDEs and with phpDocumentor; @return $this is a synonym that is less supported. If PHPStorm is not accepting it, we should raise a ticket with them.

library/Zend/Config/AbstractConfigFactory.php
((71 lines not shown))
+ * @return string|mixed|array
+ */
+ public function createServiceWithName(ServiceManager\ServiceLocatorInterface $serviceLocator, $name, $requestedName)
+ {
+ if (isset($this->configs[$requestedName])) {
+ return $this->configs[$requestedName];
+ }
+
+ $key = $this->match($requestedName);
+ if (isset($this->configs[$key])) {
+ $this->configs[$requestedName] = $this->configs[$key];
+ return $this->configs[$key];
+ }
+
+ $config = $serviceLocator->get('Config');
+ $config = new Config($config[$key]);
Matthew Weier O'Phinney Owner

As noted in a previous comment -- don't cast this to a Config object; otherwise, there may be issues with existing code that works with configuration and expects an array (the current Config service is actually an array, not a Config instance!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Chris Raidler

Rgr, will correct with the next commit.

Btw:
The fact that the Config service actually is an array is the issue why I originally came up with this. This service (allthough the default sm behaviour is shared) is imo not a shared service because retrieving the Config service from the sm returns me just another copy of the default config array rather than a reference to it (or does sm check if the service is an array and return a reference - I might check on this ...). So I was using such an abstract factory to at least dont copy my own array configs around a million times and rather work with the beauty named Zend\Config\Config :)

But I might miss the bigger picture here. If you still see a useful benefit in having this returning arrays I am happy with it.

Chris Raidler

@return self is the accepted standard, and works across IDEs and with phpDocumentor; @return $this is a synonym that is less supported. If PHPStorm is not accepting it, we should raise a ticket with them.

Thanks for the clarification. PHPStorm in fact supports @return self - my mistake.

Matthew Weier O'Phinney weierophinney merged commit 3bdfebf into from
Chris Raidler sargech deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 20, 2013
  1. added an abstract factory for configs

    Chris Raidler authored
Commits on Aug 20, 2013
  1. added abstract factory test

    Chris Raidler authored
  2. wrong key ...

    Chris Raidler authored
Commits on Aug 21, 2013
  1. Chris Raidler

    added some more functionality

    Chris Raidler authored gi-cra committed
  2. Chris Raidler

    fixed assertion and empty line at file ending

    Chris Raidler authored gi-cra committed
  3. Chris Raidler

    fix more assertions and more file endings ...

    Chris Raidler authored gi-cra committed
  4. Chris Raidler

    fix assertion

    Chris Raidler authored gi-cra committed
  5. Chris Raidler

    fix data provider

    Chris Raidler authored gi-cra committed
  6. Chris Raidler

    fix data provider

    gi-cra authored
  7. Chris Raidler

    incorporated feedback from @weierophinney

    gi-cra authored Chris Raidler committed
  8. Chris Raidler

    removed instantiation of Zend\Config\Config, adjusted test

    gi-cra authored Chris Raidler committed
  9. replaced @return $this with @return self

    Chris Raidler authored
This page is out of date. Refresh to see the latest.
172 library/Zend/Config/AbstractConfigFactory.php
View
@@ -0,0 +1,172 @@
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace Zend\Config;
+
+use Traversable;
+use Zend\ServiceManager;
+
+/**
+ * Class AbstractConfigFactory
+ */
+class AbstractConfigFactory implements ServiceManager\AbstractFactoryInterface
+{
+ /**
+ * @var array
+ */
+ protected $configs = array();
+
+ /**
+ * @var string[]
+ */
+ protected $defaultPatterns = array(
+ '#config[\._-](.*)$#i',
+ '#^(.*)[\\\\\._-]config$#i'
+ );
+
+ /**
+ * @var string[]
+ */
+ protected $patterns;
+
+ /**
+ * Determine if we can create a service with name
+ *
+ * @param ServiceManager\ServiceLocatorInterface $serviceLocator
+ * @param string $name
+ * @param string $requestedName
+ * @return bool
+ */
+ public function canCreateServiceWithName(ServiceManager\ServiceLocatorInterface $serviceLocator, $name, $requestedName)
+ {
+ if (isset($this->configs[$requestedName])) {
+ return true;
+ }
+
+ if (!$serviceLocator->has('Config')) {
+ return false;
+ }
+
+ $key = $this->match($requestedName);
+ if (null === $key) {
+ return false;
+ }
+
+ $config = $serviceLocator->get('Config');
+ return isset($config[$key]);
+ }
+
+ /**
+ * Create service with name
+ *
+ * @param ServiceManager\ServiceLocatorInterface $serviceLocator
+ * @param string $name
+ * @param string $requestedName
+ * @return string|mixed|array
+ */
+ public function createServiceWithName(ServiceManager\ServiceLocatorInterface $serviceLocator, $name, $requestedName)
+ {
+ if (isset($this->configs[$requestedName])) {
+ return $this->configs[$requestedName];
+ }
+
+ $key = $this->match($requestedName);
+ if (isset($this->configs[$key])) {
+ $this->configs[$requestedName] = $this->configs[$key];
+ return $this->configs[$key];
+ }
+
+ $config = $serviceLocator->get('Config');
+ $this->configs[$requestedName] = $this->configs[$key] = $config[$key];
+ return $config;
+ }
+
+ /**
+ * @param string $pattern
+ * @return self
+ * @throws Exception\InvalidArgumentException
+ */
+ public function addPattern($pattern)
+ {
+ if (!is_string($pattern)) {
+ throw new Exception\InvalidArgumentException('pattern must be string');
+ }
+
+ $patterns = $this->getPatterns();
+ array_unshift($patterns, $pattern);
+ $this->setPatterns($patterns);
+ return $this;
+ }
+
+ /**
+ * @param array|Traversable $patterns
+ * @return self
+ * @throws Exception\InvalidArgumentException
+ */
+ public function addPatterns($patterns)
+ {
+ if ($patterns instanceof Traversable) {
+ $patterns = iterator_to_array($patterns);
+ }
+
+ if (!is_array($patterns)) {
+ throw new Exception\InvalidArgumentException("patterns must be array or Traversable");
+ }
+
+ foreach ($patterns as $pattern) {
+ $this->addPattern($pattern);
+ }
+
+ return $this;
+ }
+
+ /**
+ * @param array|Traversable $patterns
+ * @return self
+ * @throws \InvalidArgumentException
+ */
+ public function setPatterns($patterns)
+ {
+ if ($patterns instanceof Traversable) {
+ $patterns = iterator_to_array($patterns);
+ }
+
+ if (!is_array($patterns)) {
+ throw new \InvalidArgumentException("patterns must be array or Traversable");
+ }
+
+ $this->patterns = $patterns;
+ return $this;
+ }
+
+ /**
+ * @return array
+ */
+ public function getPatterns()
+ {
+ if (null === $this->patterns) {
+ $this->setPatterns($this->defaultPatterns);
+ }
+ return $this->patterns;
+ }
+
+ /**
+ * @param string $requestedName
+ * @return null|string
+ */
+ protected function match($requestedName)
+ {
+ foreach ($this->getPatterns() as $pattern) {
+ if (preg_match($pattern, $requestedName, $matches)) {
+ return $matches[1];
+ }
+ }
+ return null;
+ }
+}
130 tests/ZendTest/Config/AbstractFactoryTest.php
View
@@ -0,0 +1,130 @@
+<?php
+/**
+ * AbstractFactoryTest.php
+ *
+ * @author Chris Raidler <chris@raidler.com>
+ * @copyright Copyright 2012 - 2013, raidler dot com
+ */
+namespace ZendTest\Config;
+
+use Zend\Config\AbstractConfigFactory;
+use Zend\Mvc\Service\ServiceManagerConfig;
+use Zend\ServiceManager;
+
+/**
+ * Class AbstractFactoryTest
+ */
+class AbstractFactoryTest extends \PHPUnit_Framework_TestCase
+{
+ /**
+ * @var \Zend\Mvc\Application
+ */
+ protected $application;
+
+ /**
+ * @var \Zend\ServiceManager\ServiceManager
+ */
+ protected $serviceManager;
+
+ /**
+ * @return void
+ */
+ public function setUp()
+ {
+ $config = array(
+ 'MyModule' => array(
+ 'foo' => array(
+ 'bar'
+ )
+ ),
+ 'phly-blog' => array(
+ 'foo' => array(
+ 'bar'
+ )
+ )
+ );
+
+ $sm = $this->serviceManager = new ServiceManager\ServiceManager(
+ new ServiceManagerConfig(array(
+ 'abstract_factories' => array(
+ 'Zend\Config\AbstractConfigFactory',
+ )
+ ))
+ );
+
+ $sm->setService('Config', $config);
+ }
+
+ /**
+ * @expectedException InvalidArgumentException
+ * @return void
+ */
+ public function testInvalidPattern()
+ {
+ $factory = new AbstractConfigFactory();
+ $factory->addPattern(new \stdClass());
+ }
+
+ /**
+ * @expectedException InvalidArgumentException
+ * @return void
+ */
+ public function testInvalidPatternIterator()
+ {
+ $factory = new AbstractConfigFactory();
+ $factory->addPatterns('invalid');
+ }
+
+ /**
+ * @return void
+ */
+ public function testPatterns()
+ {
+ $factory = new AbstractConfigFactory();
+ $defaults = $factory->getPatterns();
+
+ // Tests that the accessor returns an array
+ $this->assertInternalType('array', $defaults);
+ $this->assertGreaterThan(0, count($defaults));
+
+ // Tests adding a single pattern
+ $this->assertSame($factory, $factory->addPattern('#foobarone#i'));
+ $this->assertCount(count($defaults) + 1, $factory->getPatterns());
+
+ // Tests adding multiple patterns at once
+ $patterns = $factory->getPatterns();
+ $this->assertSame($factory, $factory->addPatterns(array('#foobartwo#i', '#foobarthree#i')));
+ $this->assertCount(count($patterns) + 2, $factory->getPatterns());
+
+ // Tests whether the latest added pattern is the first in stack
+ $patterns = $factory->getPatterns();
+ $this->assertSame('#foobarthree#i', $patterns[0]);
+ }
+
+ /**
+ * @return void
+ */
+ public function testCanCreateService()
+ {
+ $factory = new AbstractConfigFactory();
+ $serviceLocator = $this->serviceManager;
+
+ $this->assertFalse($factory->canCreateServiceWithName($serviceLocator, 'mymodulefail', 'MyModule\Fail'));
+ $this->assertTrue($factory->canCreateServiceWithName($serviceLocator, 'mymoduleconfig', 'MyModule\Config'));
+ }
+
+ /**
+ * @depends testCanCreateService
+ * @return void
+ */
+ public function testCreateService()
+ {
+ $serviceLocator = $this->serviceManager;
+ $this->assertInternalType('array', $serviceLocator->get('MyModule\Config'));
+ $this->assertInternalType('array', $serviceLocator->get('MyModule_Config'));
+ $this->assertInternalType('array', $serviceLocator->get('Config.MyModule'));
+ $this->assertInternalType('array', $serviceLocator->get('phly-blog.config'));
+ $this->assertInternalType('array', $serviceLocator->get('phly-blog-config'));
+ $this->assertInternalType('array', $serviceLocator->get('config-phly-blog'));
+ }
+}
Something went wrong with that request. Please try again.