Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Сonfigurable factories #128

Closed
skar opened this issue Jun 19, 2016 · 15 comments

Comments

@skar
Copy link

commented Jun 19, 2016

Hi, all,

I have tourbles with tons of factories (controllers/actions/simple services/etc) and I think to do simple dependencies configuration like:

'dep-name' => [
    'class' => MyService::class,
    'deps' => [
        Dep1::class,
        Dep2::class,
    ],
],

It's will be alternative for factories, invokables etc and works like a factory sample:

'dep-name' => function($container) {
    return new MyService($container->get(Dep1::class), $container->get(Dep2::class));
},

In the future it can be used for annotations/reflection support maybe.

I want to implement it.

But I need help from you!
First, I need discussions and ideas for implementation (config examples, naming etc).
Second, I need support (moral).

@Ocramius

This comment has been minimized.

Copy link
Member

commented Jun 19, 2016

As discussed on the mailing list, this is a good idea, but:

  • we need to enforce string keys for the parameters, as it is impossible to override them otherwise (merged config)

  • we need to type-check all parameters, and throw meaningful exceptions in case of problems

  • we may (further down the line) compile these definitions into factories

  • we need benchmarks for this, even if the first version will not actually be performing very well

  • we may look at symfony/dependency-injection to avoid writing our own incompatible DSL for no good reason

    In the future it can be used for annotations/reflection support maybe.

Avoid this bit: has been only pain so far ;-)

@skar

This comment has been minimized.

Copy link
Author

commented Jun 19, 2016

we need to enforce string keys for the parameters, as it is impossible to override them otherwise (merged config)

Stdlib\ArrayUtils::merge can merge numeric keys without override, but not array_merge.
I think it will be the will of the end user, but it should be described in the documentation.

we need to type-check all parameters, and throw meaningful exceptions in case of problems

Maybe typehint can help us?

we need benchmarks for this, even if the first version will not actually be performing very well

Any examples?

Avoid this bit: has been only pain so far ;-)

Keyword 'maybe' =)

@Koopzington

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2016

type-check parameters?
If the constructor of the Service has typehinted it's parameters you'd automatically get meaningful errors when trying to pass instances to it that aren't what the constructors expect.

final class Dep2
{
}

final class MyService
{
    public function __construct(Dep1 $dep1, Dep2 $dep2)
    {
    }
}

$myService = new MyService(new Dep2(), new Dep3());

Fatal error: Uncaught TypeError: Argument 1 passed to MyService::__construct() must be an instance of Dep1, instance of Dep2 given

Fatal error: Uncaught Error: Class 'Dep3' not found

Aren't those meaningful enough?

@GeeH

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2016

This is a good idea.

@GeeH

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2016

Didn't mean to enter then, I would suggest the dependencies should be service manager keys?

@GeeH GeeH added the enhancement label Aug 24, 2016

@GeeH GeeH self-assigned this Aug 24, 2016

@skar

This comment has been minimized.

Copy link
Author

commented Aug 24, 2016

Wait a minute, how about Zend Di? =)
As explained in the mailing list Zend SM - factories, Zend Di - configs.
And it's functionality must be in Zend Di.

@tasmaniski

This comment has been minimized.

Copy link

commented Aug 24, 2016

Great feature! This is exactly what will solve 90% factories in my projects.

I would suggest the dependencies should be service manager keys

Is there any reason not to work with other keys as
controllers, view_helpers or controller_plugins?

Although, feature doesn't need to be an integral part of the SM and abstract factory above is very good replacement, I would like to see this feature as part of the SM (and in it's documentation).

@GeeH

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

@skar can you provide a link to mailing list discussion please?

@skar

This comment has been minimized.

Copy link
Author

commented Aug 25, 2016

@GeeH http://zend-framework-community.634137.n4.nabble.com/Simple-zend-servicemanager-td4662889.html

And invokables was removed from SM (replaced by InvokableFactory).
After all, I think we must use Zend\Di for 'configurable factories'.

@Ocramius what do you think?

@GeeH

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

And invokables was removed from SM (replaced by InvokableFactory).

I don't understand what point this brings to the discussion.

After all, I think we must use Zend\Di for 'configurable factories'.

Is Zend\Di still even a thing? I propose we drop Zend\Di and mark it as unmaintained even if it is not already. This is an optional AbstractFactory that can be enabled on a per-project basis, has zero overhead if not enabled and no perceivable performance difference if it is enabled. It depends on the existing ServiceManager (or at least a container-interop container) to find it's dependencies. For me, it's place is where it is, in the ServiceManager namespace.

@GeeH

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

controllers, view_helpers or controller_plugins?

Current implementation it only works for the service locator the abstract factory is added to.

@skar

This comment has been minimized.

Copy link
Author

commented Aug 25, 2016

I don't understand what point this brings to the discussion.

Even this config was replaced by factory.

AbstractFactories has unnecessary overhead for this task.

@tasmaniski

This comment has been minimized.

Copy link

commented Aug 25, 2016

Is Zend\Di still even a thing? I propose we drop Zend\Di and mark it as unmaintained even if it is not already.

I too have had the same thought.

It is possible to register abstract factory for controllers also,
only different in getServiceLocator()

$serviceLocator->getServiceLocator()->get('config');

For SM V3 regular factory should be fine (instead of abstract factory).
There will be more things in config but only one factory for all services.
That should bring some performance too.

@Koopzington

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

Can't we extend the ServiceManager to parse a new key like... idk 'configurables'?
Since all PluginManagers extend the ServiceManager you could then have those 'configurables' in 'service_manager', 'view_helpers', 'controller_plugins', etc.
If the config has that key then add an ConfigAbstractFactory to the PluginManager and pass the defined config to that ConfigAbstractFactory's constructor instead of letting it grab the merged config from the Container.

@weierophinney

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

Resolved with #146, to be released soon with 3.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.