New LazyControllerFactory #165

Merged
merged 9 commits into from Jun 23, 2016

Projects

None yet

3 participants

@weierophinney
Member

Inspired by http://circlical.com/blog/2016/3/9/preparing-for-zend-f, this abstract factory/factory can be used to create controller instances for controllers defining constructor dependencies, using the following rules:

  • A parameter named $config typehinted as an array will receive the application "config" service (i.e., the merged configuration).
  • Parameters type-hinted against array, but not named $config will be injected with an empty array.
  • Scalar parameters will be resolved as null values.
  • If a service cannot be found for a given typehint, the factory will raise an exception detailing this.
  • Some services provided by Zend Framework components do not have entries based on their class name (for historical reasons); the factory contains a map of these class/interface names to the corresponding service name to allow them to resolve.

$options passed to the factory are ignored in all cases, as we cannot make assumptions about which argument(s) they might replace.

As it implements zend-servicemanager's v3 AbstractFactoryInterface, it may be used as either an abstract factory, or by mapping controller class names to the factory.

@weierophinney weierophinney New LazyControllerFactory
Inspired by http://circlical.com/blog/2016/3/9/preparing-for-zend-f,
this abstract factory/factory can be used to create controller
instances for controllers defining constructor dependencies, using
the following rules:

- A parameter named `$config` typehinted as an array will receive the
  application "config" service (i.e., the merged configuration).
- Parameters type-hinted against array, but not named `$config` will
  be injected with an empty array.
- Scalar parameters will be resolved as null values.
- If a service cannot be found for a given typehint, the factory will
  raise an exception detailing this.
- Some services provided by Zend Framework components do not have
  entries based on their class name (for historical reasons); the
  factory contains a map of these class/interface names to the
  corresponding service name to allow them to resolve.

`$options` passed to the factory are ignored in all cases, as we cannot
make assumptions about which argument(s) they might replace.

As it implements zend-servicemanager's v3 AbstractFactoryInterface, it
may be used as either an abstract factory, or by mapping controller
class names to the factory.
8e2e226
@weierophinney weierophinney added this to the 3.0.1 milestone Jun 23, 2016
@Ocramius Ocramius and 2 others commented on an outdated diff Jun 23, 2016
src/Controller/LazyControllerFactory.php
+ * corresponding service name to allow them to resolve.
+ *
+ * `$options` passed to the factory are ignored in all cases, as we cannot
+ * make assumptions about which argument(s) they might replace.
+ */
+class LazyControllerFactory implements AbstractFactoryInterface
+{
+ /**
+ * Maps known classes/interfaces to the service that provides them; only
+ * required for those services with no entry based on the class/interface
+ * name.
+ *
+ * @var string[]
+ */
+ private $aliases = [
+ 'Zend\Console\Adapter\AdapterInterface' => 'ConsoleAdapter',
@Ocramius
Ocramius Jun 23, 2016 Member

::class all these, plz

@snapshotpl
snapshotpl Jun 23, 2016 Contributor

::class need to install all repositories, because need this classes

@Ocramius
Ocramius Jun 23, 2016 Member

No, ::class does not cause autoloading.

@weierophinney
weierophinney Jun 23, 2016 Member

@snapshotpl Not true; PHP will resolve the class name using ::class even if it cannot autoload the class.

@Ocramius
Ocramius Jun 23, 2016 Member

You can use ::class even with non-existing classes

@snapshotpl
snapshotpl Jun 23, 2016 Contributor

Interesting. So Zend\Console\Adapter\AdapterInterface::class will be Zend\Console\Adapter\AdapterInterface string even I autoload it or not?

@Ocramius Ocramius commented on an outdated diff Jun 23, 2016
src/Controller/LazyControllerFactory.php
+ * - A parameter named `$config` typehinted as an array will receive the
+ * application "config" service (i.e., the merged configuration).
+ * - Parameters type-hinted against array, but not named `$config` will
+ * be injected with an empty array.
+ * - Scalar parameters will be resolved as null values.
+ * - If a service cannot be found for a given typehint, the factory will
+ * raise an exception detailing this.
+ * - Some services provided by Zend Framework components do not have
+ * entries based on their class name (for historical reasons); the
+ * factory contains a map of these class/interface names to the
+ * corresponding service name to allow them to resolve.
+ *
+ * `$options` passed to the factory are ignored in all cases, as we cannot
+ * make assumptions about which argument(s) they might replace.
+ */
+class LazyControllerFactory implements AbstractFactoryInterface
@Ocramius
Ocramius Jun 23, 2016 Member

s/Factory/AbstractFactory

@Ocramius Ocramius commented on an outdated diff Jun 23, 2016
src/Controller/LazyControllerFactory.php
+ $parameters[] = $container->get($type);
+ }
+
+ return $reflectionClass->newInstanceArgs($parameters);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function canCreate(ContainerInterface $container, $requestedName)
+ {
+ if (! is_string($requestedName) || ! class_exists($requestedName)) {
+ return false;
+ }
+
+ $implements = class_implements($requestedName);
@Ocramius
Ocramius Jun 23, 2016 Member

No need for this assignment

@Ocramius Ocramius and 1 other commented on an outdated diff Jun 23, 2016
src/Controller/LazyControllerFactory.php
+ $type
+ ));
+ }
+
+ $parameters[] = $container->get($type);
+ }
+
+ return $reflectionClass->newInstanceArgs($parameters);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ public function canCreate(ContainerInterface $container, $requestedName)
+ {
+ if (! is_string($requestedName) || ! class_exists($requestedName)) {
@Ocramius
Ocramius Jun 23, 2016 Member

can $requestedName ever be a non-string?

@weierophinney
weierophinney Jun 23, 2016 Member

I've seen null a few times, though primarily when used in a servicemanager v2 context. I'll remove that check.

@weierophinney
weierophinney Jun 23, 2016 Member

To clarify: zend-mvc v3 requires zend-servicemanager v3, and the class is implementing only the v3 interface. Thus, the check is superfluous.

@snapshotpl snapshotpl and 1 other commented on an outdated diff Jun 23, 2016
src/Controller/LazyControllerFactory.php
+ * factory contains a map of these class/interface names to the
+ * corresponding service name to allow them to resolve.
+ *
+ * `$options` passed to the factory are ignored in all cases, as we cannot
+ * make assumptions about which argument(s) they might replace.
+ */
+class LazyControllerFactory implements AbstractFactoryInterface
+{
+ /**
+ * Maps known classes/interfaces to the service that provides them; only
+ * required for those services with no entry based on the class/interface
+ * name.
+ *
+ * @var string[]
+ */
+ private $aliases = [
@snapshotpl
snapshotpl Jun 23, 2016 Contributor

It will be nice to provide custom aliases

@weierophinney
weierophinney Jun 23, 2016 Member

I'll make it protected so this factory can be extended, then.

@weierophinney weierophinney Use `::class` for aliased classes in LazyControllerFactory
- Per @Ocramius
4ba6e25
@snapshotpl snapshotpl commented on an outdated diff Jun 23, 2016
src/Controller/LazyControllerFactory.php
+ $type = $parameter->getClass()->getName();
+ $type = array_key_exists($type, $this->aliases) ? $this->aliases[$type] : $type;
+
+ if (! $container->has($type)) {
+ throw new ServiceNotFoundException(sprintf(
+ 'Unable to create controller "%s"; unable to resolve parameter "%s" using type hint "%s"',
+ $requestedName,
+ $parameter->getName(),
+ $type
+ ));
+ }
+
+ $parameters[] = $container->get($type);
+ }
+
+ return $reflectionClass->newInstanceArgs($parameters);
@snapshotpl snapshotpl commented on an outdated diff Jun 23, 2016
src/Controller/LazyControllerFactory.php
+ $parameters[] = $container->get('config');
+ continue;
+ }
+
+ if ($parameter->isArray()) {
+ $parameters[] = [];
+ continue;
+ }
+
+ if (! $parameter->getClass()) {
+ $parameters[] = null;
+ continue;
+ }
+
+ $type = $parameter->getClass()->getName();
+ $type = array_key_exists($type, $this->aliases) ? $this->aliases[$type] : $type;
@snapshotpl
snapshotpl Jun 23, 2016 Contributor

isset is faster, right?

@weierophinney weierophinney Made $aliases protected (not private)
- Allows extending in order to add more aliases.
dd34bad
@snapshotpl snapshotpl commented on the diff Jun 23, 2016
src/Controller/LazyControllerAbstractFactory.php
+ * - Some services provided by Zend Framework components do not have
+ * entries based on their class name (for historical reasons); the
+ * factory contains a map of these class/interface names to the
+ * corresponding service name to allow them to resolve.
+ *
+ * `$options` passed to the factory are ignored in all cases, as we cannot
+ * make assumptions about which argument(s) they might replace.
+ */
+class LazyControllerAbstractFactory implements AbstractFactoryInterface
+{
+ /**
+ * Maps known classes/interfaces to the service that provides them; only
+ * required for those services with no entry based on the class/interface
+ * name.
+ *
+ * Extend the class if you wish to add to the list.
@snapshotpl
snapshotpl Jun 23, 2016 Contributor

Can't we get this list from configuration?

@weierophinney
weierophinney Jun 23, 2016 Member

I'd rather not add a configuration point for it. This currently lists all "special case" service names, and we recommend that users utilize fully qualified interface and/or class names for their service names.

If a user really wants to add more, they should have to think twice before doing so.

@snapshotpl
snapshotpl Jun 23, 2016 edited Contributor

No I see that https://github.com/zendframework/zend-mvc/pull/165/files#diff-898a3a94db9fcef93eeeb26a23397ecaR139 will resolve many cases. Anyway this feature it's for really lazy developers, and never will be cover all cases

@weierophinney
weierophinney Jun 23, 2016 Member

Precisely. If you need something more robust, or with more features, you should likely create a custom factory at that point, not extend this one. ๐Ÿ˜„

@weierophinney weierophinney Use splat operator
- Updated instantiation to use splat operator instead of reflection
- switch from array_key_exists to isset

Both per @snapshotpl
bc75c3d
@weierophinney
Member

@snapshotpl Thanks for reminding me about the splat operator; still not used to being able to use PHP 5.6 features!

@snapshotpl
Contributor

That's why we goes into 5.6!, right?

@weierophinney weierophinney Refactor: extract method
Extracted parameter resolution to a new method, and modified factory to
use array_map with that method to create the list of parameters for
instantiating the controller.
7d30314
@snapshotpl snapshotpl and 1 other commented on an outdated diff Jun 23, 2016
src/Controller/LazyControllerAbstractFactory.php
+ */
+ public function __invoke(ContainerInterface $container, $requestedName, array $options = null)
+ {
+ $reflectionClass = new ReflectionClass($requestedName);
+
+ if (null === ($constructor = $reflectionClass->getConstructor())) {
+ return new $requestedName();
+ }
+
+ $reflectionParameters = $constructor->getParameters();
+
+ if (empty($reflectionParameters)) {
+ return new $requestedName();
+ }
+
+ $parameters = array_map(function (ReflectionParameter $parameter) use ($container, $requestedName) {
@snapshotpl
snapshotpl Jun 23, 2016 Contributor

I read about performance array_map and foreach and looks a little bit slower. use also decrease readability for me. Maybe foreach will be better?

@weierophinney
weierophinney Jun 23, 2016 Member

Micro-optimizations with the size of the arrays we have here. But I agree on the readability part; really wish you could do callables using private methods if invoked within the context of the class.

@weierophinney
weierophinney Jun 23, 2016 Member

I've refactored this now. resolveParameter() now returns a closure over the container and requested name, leaving:

$parameters = array_map(
    $this->resolveParameter($container, $requestedName),
    $reflectionParameters
);
@weierophinney weierophinney Refactor array_map
`resolveParameter()` now accepts the container and requested name, and
returns a callback for resolving a parameter to a value, closing over
the two provided values. This simplifies the array_map call, making it
more readable.
00ba958
@weierophinney weierophinney merged commit 00ba958 into zendframework:master Jun 23, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 83.083%
Details
@weierophinney weierophinney added a commit that referenced this pull request Jun 23, 2016
@weierophinney weierophinney Added CHANGELOG for #165 6417097
@weierophinney weierophinney deleted the weierophinney:feature/lazy-service-controller-factory branch Jun 23, 2016
@Ocramius
Member

@weierophinney I just realized that this PR has nothing to do with laziness, but is instead centered on "automatic di". Should I open an issue about it, or can you still work on it?

@weierophinney
Member

@ocramius โ€” What's your idea? A rename? Or a more generic offering in
zend--servicemanager?
On Jun 23, 2016 7:39 PM, "Marco Pivetta" notifications@github.com wrote:

@weierophinney https://github.com/weierophinney I just realized that
this PR has nothing to do with laziness, but is instead centered on
"automatic di". Should I open an issue about it, or can you still work on
it?

โ€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#165 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABlV0WWRFtIntt4GKtWu80bvyqAgGISks5qOyddgaJpZM4I9CNX
.

@Ocramius
Member

A rename is sufficient. Just want to avoid messy situations with
documentation later on
On Jun 24, 2016 03:49, "weierophinney" notifications@github.com wrote:

@ocramius โ€” What's your idea? A rename? Or a more generic offering in
zend--servicemanager?
On Jun 23, 2016 7:39 PM, "Marco Pivetta" notifications@github.com wrote:

@weierophinney https://github.com/weierophinney I just realized that
this PR has nothing to do with laziness, but is instead centered on
"automatic di". Should I open an issue about it, or can you still work on
it?

โ€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<
#165 (comment)
,
or mute the thread
<
https://github.com/notifications/unsubscribe/AABlV0WWRFtIntt4GKtWu80bvyqAgGISks5qOyddgaJpZM4I9CNX

.

โ€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#165 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAJakDtk9DVdI5rx6WcKvAaW1Ch_8r7Iks5qOzengaJpZM4I9CNX
.

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