Skip to content
This repository has been archived by the owner on Feb 6, 2020. It is now read-only.

Allows the use of ArrayObject config service with ConfigAbstractFactory #174

Conversation

bfolliot
Copy link
Contributor

No description provided.

@bfolliot
Copy link
Contributor Author

With Expressive skeleton, "config" service is an ArrayObject https://github.com/zendframework/zend-expressive-skeleton/blob/master/config/config.php

@geerteltink
Copy link
Member

The ArrayObject is caused by using the zend-expressive-skeleton to setup the application. It is only there for the Aura.Di and is fixed and removed in the develop branch.

You can easily replace that last line in config/config.php with return $config;.

If this causes issues in a zend-expressive-skeleton based application, I think it should be reported to zend-expressive-skeleton. So far no issues have been reported. It would be interesting to know what steps you took during installation in order to get that error.

@bfolliot
Copy link
Contributor Author

Yes, but i think it may be good to have this compatibility with ArrayObject.

Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

Definitely feel this is a necessary addition; we've vacillated between ArrayObject and array for the config service over the years, and the main point is that the service acts like an array.

I've noted a few changes below.

@@ -40,8 +41,8 @@ public function __invoke(\Interop\Container\ContainerInterface $container, $requ

$config = $container->get('config');

if (! is_array($config)) {
throw new ServiceNotCreatedException('Config must be an array');
if (! is_array($config) and (! is_object($config) or ! $config instanceof \ArrayObject)) {
Copy link
Member

Choose a reason for hiding this comment

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

Change this line to:

if (! (is_array($config) || $config instanceof ArrayObject)) {

]
])
);
self::assertTrue($abstractFactory->canCreate($serviceManager, InvokableObject::class));
Copy link
Member

Choose a reason for hiding this comment

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

Remove this. This particular test case is covering the values related to configured classes. You have a more specific test for the ArrayObject behavior below which this is duplicating anyways.

@@ -76,6 +88,23 @@ public function testCanCreate()
self::assertFalse($abstractFactory->canCreate($serviceManager, ServiceManager::class));
}

public function testCanCreateWithArrayObject()
Copy link
Member

Choose a reason for hiding this comment

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

Rename this to testCanCreateReturnsTrueWhenConfigIsAnArrayObject.

Copy link
Member

Choose a reason for hiding this comment

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

Also, add an additional test testFactoryCanCreateInstancesWhenConfigIsAnArrayObject, demonstrating that the factory correctly creates the instance when the config service is an ArrayObject.

@bfolliot bfolliot force-pushed the ConfigAbstractFactory_options-from-array-object branch from 41ee165 to 7efae58 Compare January 24, 2017 14:42
@bfolliot
Copy link
Contributor Author

@weierophinney : It's good for you now?

@weierophinney weierophinney added this to the 3.2.1 milestone Feb 15, 2017
@weierophinney weierophinney self-assigned this Feb 15, 2017
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

Changes look good!

@weierophinney weierophinney merged commit 7efae58 into zendframework:master Feb 15, 2017
weierophinney added a commit that referenced this pull request Feb 15, 2017
…rom-array-object

Allows the use of ArrayObject config service with ConfigAbstractFactory
weierophinney added a commit that referenced this pull request Feb 15, 2017
weierophinney added a commit that referenced this pull request Feb 15, 2017
weierophinney added a commit that referenced this pull request Feb 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants