abstract factory for configs reading keys from merged config #4860

Merged
merged 13 commits into from Oct 22, 2013

Conversation

Projects
None yet
2 participants
@ghost

ghost commented Jul 20, 2013

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.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 20, 2013

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

ghost commented Jul 20, 2013

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

@ghost

View changes

library/Zend/Config/AbstractConfigFactory.php
+ /**
+ * @var string
+ */
+ protected $pattern = '#^(.*)\\\\Config$#i';

This comment has been minimized.

@ghost

ghost Jul 20, 2013

this could be done more performant using strrpos

@ghost

ghost Jul 20, 2013

this could be done more performant using strrpos

This comment has been minimized.

@weierophinney

weierophinney Aug 20, 2013

Member

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.

@weierophinney

weierophinney Aug 20, 2013

Member

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.

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Aug 19, 2013

Member

Please add unit tests, @sarge-ch

Member

weierophinney commented Aug 19, 2013

Please add unit tests, @sarge-ch

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 20, 2013

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.

ghost commented Aug 20, 2013

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.

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Aug 20, 2013

Member

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

Member

weierophinney commented Aug 20, 2013

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

@weierophinney

View changes

library/Zend/Config/AbstractConfigFactory.php
+ return false;
+ }
+
+ $config = $serviceLocator->get('Config');

This comment has been minimized.

@weierophinney

weierophinney Aug 20, 2013

Member

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.

@weierophinney

weierophinney Aug 20, 2013

Member

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.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 21, 2013

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

ghost commented Aug 21, 2013

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

@weierophinney

View changes

library/Zend/Config/AbstractConfigFactory.php
+ public function setPatterns($patterns)
+ {
+ if ($patterns instanceof \Traversable) {
+ $patterns = ArrayUtils::iteratorToArray($patterns);

This comment has been minimized.

@weierophinney

weierophinney Aug 21, 2013

Member

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

@weierophinney

weierophinney Aug 21, 2013

Member

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

@weierophinney

View changes

library/Zend/Config/AbstractConfigFactory.php
+ }
+
+ if (!is_array($patterns)) {
+ throw new \InvalidArgumentException("patterns must be array or Traversable");

This comment has been minimized.

@weierophinney

weierophinney Aug 21, 2013

Member

Use the exceptions from the component. :)

(I'll make this change on merge)

@weierophinney

weierophinney Aug 21, 2013

Member

Use the exceptions from the component. :)

(I'll make this change on merge)

@weierophinney

View changes

library/Zend/Config/AbstractConfigFactory.php
+ */
+ public function setPatterns($patterns)
+ {
+ if ($patterns instanceof \Traversable) {

This comment has been minimized.

@weierophinney

weierophinney Aug 21, 2013

Member

Import Traversable.

@weierophinney

weierophinney Aug 21, 2013

Member

Import Traversable.

@weierophinney

View changes

library/Zend/Config/AbstractConfigFactory.php
+
+ /**
+ * @param array|\Traversable $patterns
+ * @return $this

This comment has been minimized.

@weierophinney

weierophinney Aug 21, 2013

Member

s/$this/self/

@weierophinney

weierophinney Aug 21, 2013

Member

s/$this/self/

@weierophinney

View changes

library/Zend/Config/AbstractConfigFactory.php
+ {
+ $key = $this->match($requestedName);
+ $config = $serviceLocator->get('Config');
+ return new Config($config[$key]);

This comment has been minimized.

@weierophinney

weierophinney Aug 21, 2013

Member

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

@weierophinney

weierophinney Aug 21, 2013

Member

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

This comment has been minimized.

@weierophinney

weierophinney Aug 21, 2013

Member

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.

@weierophinney

weierophinney Aug 21, 2013

Member

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.

@weierophinney

View changes

library/Zend/Config/AbstractConfigFactory.php
+ * @param ServiceManager\ServiceLocatorInterface $serviceLocator
+ * @param string $name
+ * @param string $requestedName
+ * @return mixed

This comment has been minimized.

@weierophinney

weierophinney Aug 21, 2013

Member

s/mixed/array/

@weierophinney

weierophinney Aug 21, 2013

Member

s/mixed/array/

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Aug 21, 2013

Member

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

Member

weierophinney commented Aug 21, 2013

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

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 21, 2013

@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.

ghost commented Aug 21, 2013

@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.

@weierophinney

This comment has been minimized.

Show comment
Hide comment
@weierophinney

weierophinney Aug 21, 2013

Member

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.

Member

weierophinney commented Aug 21, 2013

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.

@weierophinney

View changes

library/Zend/Config/AbstractConfigFactory.php
+ }
+
+ $config = $serviceLocator->get('Config');
+ $config = new Config($config[$key]);

This comment has been minimized.

@weierophinney

weierophinney Aug 21, 2013

Member

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!).

@weierophinney

weierophinney Aug 21, 2013

Member

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!).

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 21, 2013

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.

ghost commented Aug 21, 2013

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.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Aug 21, 2013

@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.

ghost commented Aug 21, 2013

@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.

@ghost ghost assigned weierophinney Oct 22, 2013

weierophinney added a commit that referenced this pull request Oct 22, 2013

Merge pull request #4860 from sarge-ch/abstract-config-factory
abstract factory for configs reading keys from merged config

weierophinney added a commit that referenced this pull request Oct 22, 2013

@weierophinney weierophinney merged commit 3bdfebf into zendframework:develop Oct 22, 2013

1 check failed

default The Travis CI build could not complete due to an error
Details

@ghost ghost deleted the abstract-config-factory branch Oct 22, 2013

weierophinney added a commit to zendframework/zend-config that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#4860 from sarge-ch/abs…
…tract-config-factory

abstract factory for configs reading keys from merged config

weierophinney added a commit to zendframework/zend-config that referenced this pull request May 15, 2015

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