abstract factory for configs reading keys from merged config

[ghost]

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]

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

[ghost]

this could be done more performant using strrpos

[weierophinney]

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]

Please add unit tests, @sarge-ch

[ghost]

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]

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

[weierophinney]

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]

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

[weierophinney]

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

[weierophinney]

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

[ghost]

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

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]

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]

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]

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