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

Alternative autowiring approach [WIP] #84

Merged
merged 1 commit into from May 20, 2016
Merged

Alternative autowiring approach [WIP] #84

merged 1 commit into from May 20, 2016

Conversation

@dg
Copy link
Member

dg commented Sep 10, 2015

All services are autowired. setAutowired(TRUE) means that service is preferred, setAutowired(FALSE) means that is autowired too, but not preferred (yes, name setAutowired(FALSE) is weird here, but this is just experiment).

Only services that are excluded via $excludedClasses are not autowired.

Now you can create services FileStorage (autowired: on) and DevNullStorage (autowired: off) and access the first one via getByType('IStorage') and second one viagetByType('DevNullStorage')`.

I tried a different approach

@dg

This comment has been minimized.

Copy link
Member Author

dg commented Sep 10, 2015

@matej21

This comment has been minimized.

Copy link
Contributor

matej21 commented Sep 10, 2015

👍

@dg

This comment has been minimized.

Copy link
Member Author

dg commented Sep 10, 2015

I wonder how this commit affects large containers (could it try @fprochazka, @ondrejmirtes, @pilec, …)? In my projects containers are the same, only with minor difference in meta data.

@ondrejmirtes

This comment has been minimized.

Copy link
Contributor

ondrejmirtes commented Sep 10, 2015

I use setting autowired to off to exclude services from being accidentally injected somewhere where I do not want them. For example I have all instances of Monolog\Logger set to autowired: off because when I ask for a logger in a constructor, I want the DI container to fail and require specifying which concrete logger I want to inject into the service.

I am afraid that this change will lead to injecting of random unwanted services. Or is my scenario somehow solvable after this change?

@@ -327,7 +331,8 @@ public function prepareClassList()
foreach ($this->definitions as $name => $def) {
if ($class = $def->getImplement() ?: $def->getClass()) {
foreach (class_parents($class) + class_implements($class) + [$class] as $parent) {
$this->classes[$parent][$def->isAutowired() && empty($excludedClasses[$parent])][] = (string) $name;
$autowired = isset($excludedClasses[$parent]) ? NULL : $def->isAutowired();

This comment has been minimized.

Copy link
@matej21

matej21 Sep 10, 2015

Contributor

This breaks ApplicationExtension which looks for presenters by excluded class name: https://api.nette.org/2.3.5/source-Bridges.ApplicationDI.ApplicationExtension.php.html#97

This comment has been minimized.

Copy link
@dg

dg Sep 10, 2015

Author Member

findByType() is not affected.

This comment has been minimized.

Copy link
@matej21

matej21 Sep 10, 2015

Contributor

Oh, I didn't notice commit 6b24a1a :)

@dg

This comment has been minimized.

Copy link
Member Author

dg commented Sep 10, 2015

@ondrejmirtes this can be solved in two ways:

  1. option autowired will have three states: off, on and preferred (the default one), so you can disable autowiring in the same way as before
  2. option autowired will have two states: on and preferred (the default one?) and you can disable autowiring via excluded classes and they will be configurable via config.neon.

I have absolutely no idea which approach is better.

The 1) is close to current approach, but three states will make it very difficult for programmers to figure out which state they should use.

The 2) is really different, because there is not autowire: off, but it fits perfectly to your scenario. You can disable autowiring for i.e ILogger, for all services with a single line, so it is less error prone.

If people use autowired: no in sense not preferred, the 2) is fine. If they use it in sense „prevent accidental injection“, the 2) is maybe better solution too.

(Now I am not talking about back compatibility, it will remain, just about best practices.)

@fprochazka

This comment has been minimized.

Copy link
Contributor

fprochazka commented Sep 11, 2015

I've tried it and it doesn't exactly work :) But I don't have much time to dig in.

snimek obrazovky porizeny 2015-09-11 20 49 35

@MartinMystikJonas

This comment has been minimized.

Copy link

MartinMystikJonas commented Sep 14, 2015

@dg: Just idea: What abou setAutowired($priority). FALSE = not autowire, TRUE=1=autowired lowest priority, 2+=autowired with ascending priority. It would be backward compatible but sloves what I think you trying to solve.

@dg

This comment has been minimized.

Copy link
Member Author

dg commented Sep 14, 2015

@MartinMystikJonas I think it is the same as first option here #84 (comment)

@MartinMystikJonas

This comment has been minimized.

Copy link

MartinMystikJonas commented Sep 14, 2015

@dg Basically yes. But my solution can solve this for deeper inheritance hierarchies when you need to set prefferences when multiple services in inheritance hierarchies matches searched type.

For example:
Service A - IStorage, FileStorage ->setAutowire(3)
Service B - IStorage, FileStorage, MyFileStorage ->setAutowire(2)
Service C - IStorage, DevNullStorage ->setAutowire(1)

getByType('IStorage') -> Service A
getByType('FileStorage') -> Service A
getByType('MyFileStorage') -> Service B
getByType('DevNullStorage') -> Service C

@dg

This comment has been minimized.

Copy link
Member Author

dg commented Sep 14, 2015

Service A with setAutowired(2) and B & C with setAutowired(1) will have the same result, or not?

@MartinMystikJonas

This comment has been minimized.

Copy link

MartinMystikJonas commented Sep 14, 2015

You are right. My solution is only useful in even more complex situations and therefore probably not needed.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented Oct 28, 2015

👍 Just found out I need this.

@enumag

This comment has been minimized.

Copy link
Contributor

enumag commented Oct 28, 2015

One thing that I don't like in Nette/DI is overriding of autowired services by another extension. For example I've an extension which provides alternative implementation of Nette/Application/Application. Now what the extension can do to override the default implementation? In my opinion it's wrong for the custom extension to change the original service in any way (changing class/factory, turning of autowiring etc.) - but it currently can't be solved without it.

This PR doesn't solve this of course. I'm mentioning it because it could solve it (by using priorities or something). If you think it's off-topic, just ignore this comment.

@brabijan

This comment has been minimized.

Copy link

brabijan commented Nov 3, 2015

👍

@dg dg force-pushed the nette:master branch from 333d42f to 9b0f815 Nov 13, 2015
@paveljanda

This comment has been minimized.

Copy link

paveljanda commented Nov 16, 2015

👍

@dg dg force-pushed the nette:master branch from ba7b241 to fb96e4f Feb 8, 2016
@dg dg force-pushed the nette:master branch 2 times, most recently from 231a29c to 7f12a9f Apr 21, 2016
@dg dg force-pushed the nette:master branch from 839f00c to d1d4105 May 7, 2016
@dg dg force-pushed the dg:autowire-alt branch from e556e3c to 213c117 May 8, 2016
@dg

This comment has been minimized.

Copy link
Member Author

dg commented May 8, 2016

Now option autowired can contain name or names of classes/interfaces. It make services autowired only for specified types and preferably.

$defAutowired = $def->isAutowired();
if (is_array($defAutowired)) {
foreach ($defAutowired as $k => $aclass) {
if ($aclass === self::THIS_SERVICE) {

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 8, 2016

Contributor

So you can write autowire: [self]? Is that useful?

This comment has been minimized.

Copy link
@dg

dg May 8, 2016

Author Member

It is autowired only when typehint === class name.

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 8, 2016

Contributor

Yes, but in what instances would you write self instead of writing directly the class name?

Also, it's not autowired when typehint === class name but when typehint instanceof class name, or not?

$this->classList[$parent][FALSE] = array_merge(...$this->classList[$parent]);
}
$preferred[$parent] = $autowired = TRUE;
break;

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 8, 2016

Contributor

Shouldn't we throw an exception if !empty($preferred[$parent]), or even better try to resolve who should win based on distance of $parent and $aClass? Now this seems to depend order of definitions.

This comment has been minimized.

Copy link
@dg

dg May 8, 2016

Author Member

It should not rely on order, but maybe is implementation wrong, I'll check it later.

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 8, 2016

Contributor

I looked at it again and it seems to be fine, the conflict is resolved in runtime.

This comment has been minimized.

Copy link
@dg

dg May 8, 2016

Author Member

I think that there is missing line, it should erase [TRUE] list…

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 8, 2016

Contributor

You mean that the following should work?

test(function () {
    $builder = new DI\ContainerBuilder;
    $builder->addDefinition('bar')
        ->setClass('Bar')
        ->setAutowired(TRUE);
    $builder->addDefinition('foo')
        ->setClass('Foo')
        ->setAutowired('Foo');

    Assert::same('foo', $builder->getByType('Foo'));
    Assert::same('bar', $builder->getByType('Bar'));
});
@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented May 8, 2016

Because you choose the instance of approach instead of === approach, I don't understand why would you ever need to declare multiple autowire types, i.e. why support bool|array|string and not just bool|string.


But maybe I'm missing something. I had a bit of trouble reading the code.

@dg

This comment has been minimized.

Copy link
Member Author

dg commented May 8, 2016

I tried to implement a completely different solution. Option autowired can be true or false (the same meaning), or class name or list of class names.

In this case, autowiring is limited only to these classes. For example, when class implements three interfaces and I want to autowired it only for one or two of them, I will enumerate these interfaces. So when argument typehint matches of one of enumerated interfaces, service will be used for autowiring.

Therefore when I will specify it's own class name (or use autowired: self), service will be autowired only when argument typehint === specified interface.

At the same time, in addition to limiting the class list, the service becomes for that type preferred. So when there will be multiple (normally) autowired services that meet the specified class/interface, this service with specific option will win.

@dg dg force-pushed the dg:autowire-alt branch from 213c117 to efa9d25 May 8, 2016
@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented May 8, 2016

I think I finally understand it now, the getByType logic can be rewritten in pseudocode as

coalesce(
    definitions.filter(_.class instanceof type).filter(type instance of _.autowired),
    definitions.filter(_.class instanceof type).filter(_.autowired === true)
)

Making the following 8 tests cover the core logic.

test(function () {
    $builder = new DI\ContainerBuilder;
    $builder->addDefinition('bar')
        ->setClass('Bar')
        ->setAutowired('Bar');

    Assert::same('bar', $builder->getByType('Bar'));
    Assert::same(NULL, $builder->getByType('IBar'));
    Assert::same(NULL, $builder->getByType('Foo'));
    Assert::same(NULL, $builder->getByType('IFoo'));
});


test(function () {
    $builder = new DI\ContainerBuilder;
    $builder->addDefinition('bar')
        ->setClass('Bar')
        ->setAutowired('IBar');

    Assert::same('bar', $builder->getByType('Bar'));
    Assert::same('bar', $builder->getByType('IBar'));
    Assert::same(NULL, $builder->getByType('Foo'));
    Assert::same(NULL, $builder->getByType('IFoo'));
});


test(function () {
    $builder = new DI\ContainerBuilder;
    $builder->addDefinition('bar')
        ->setClass('Bar')
        ->setAutowired('Foo');

    Assert::same('bar', $builder->getByType('Bar'));
    Assert::same(NULL, $builder->getByType('IBar'));
    Assert::same('bar', $builder->getByType('Foo'));
    Assert::same(NULL, $builder->getByType('IFoo'));
});


test(function () {
    $builder = new DI\ContainerBuilder;
    $builder->addDefinition('bar')
        ->setClass('Bar')
        ->setAutowired('IFoo');

    Assert::same('bar', $builder->getByType('Bar'));
    Assert::same(NULL, $builder->getByType('IBar'));
    Assert::same('bar', $builder->getByType('Foo'));
    Assert::same('bar', $builder->getByType('IFoo'));
});


test(function () {
    $builder = new DI\ContainerBuilder;
    $builder->addDefinition('bar')
        ->setClass('Bar')
        ->setAutowired(['IFoo', 'IBar']);

    Assert::same('bar', $builder->getByType('Bar'));
    Assert::same('bar', $builder->getByType('IBar'));
    Assert::same('bar', $builder->getByType('Foo'));
    Assert::same('bar', $builder->getByType('IFoo'));
});


test(function () {
    $builder = new DI\ContainerBuilder;
    $builder->addDefinition('bar')
        ->setClass('Bar')
        ->setAutowired(['Foo', 'Bar']);

    Assert::same('bar', $builder->getByType('Bar'));
    Assert::same(NULL, $builder->getByType('IBar'));
    Assert::same('bar', $builder->getByType('Foo'));
    Assert::same(NULL, $builder->getByType('IFoo'));
});


test(function () {
    $builder = new DI\ContainerBuilder;
    $builder->addDefinition('bar')
        ->setClass('Bar')
        ->setAutowired(['Foo', 'IBar']);

    Assert::same('bar', $builder->getByType('Bar'));
    Assert::same('bar', $builder->getByType('IBar'));
    Assert::same('bar', $builder->getByType('Foo'));
    Assert::same(NULL, $builder->getByType('IFoo'));
});


test(function () {
    $builder = new DI\ContainerBuilder;
    $builder->addDefinition('bar')
        ->setClass('Bar')
        ->setAutowired(['IFoo', 'Bar']);

    Assert::same('bar', $builder->getByType('Bar'));
    Assert::same(NULL, $builder->getByType('IBar'));
    Assert::same('bar', $builder->getByType('Foo'));
    Assert::same('bar', $builder->getByType('IFoo'));
});
@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented May 8, 2016

Having a class hierarchy graph (including interfaces of course), the setClass() marks one node (green) in graph and setAutowired() marks the second node (orange). The resulting autowired types are all nodes along the path from the setClass() node to the setAutowired() node.

test(function () {
    $builder = new DI\ContainerBuilder;
    $builder->addDefinition('bar')
        ->setClass('Bar')
        ->setAutowired('IFoo');

    Assert::same('bar', $builder->getByType('Bar'));
    Assert::same(NULL, $builder->getByType('IBar'));
    Assert::same('bar', $builder->getByType('Foo'));
    Assert::same('bar', $builder->getByType('IFoo'));
});

😍

return $this;
}


/**
* @return bool
* @return bool|string[]
*/
public function isAutowired()

This comment has been minimized.

Copy link
@TomasVotruba

TomasVotruba May 8, 2016

Contributor

Method with isX name, that returns bool and string looks really confusing to me.

I'd keep preferred autowired types and TRUE/FALSE state separated.

@dg dg force-pushed the nette:master branch from 6bab21a to 631e49d May 9, 2016
@dg dg force-pushed the dg:autowire-alt branch from efa9d25 to 286167d May 9, 2016
@dg dg force-pushed the nette:master branch 7 times, most recently from 9fbd1d4 to a71f267 May 9, 2016
@dg dg force-pushed the dg:autowire-alt branch from 86b3bd8 to 6cbe9c5 May 19, 2016
@dg

This comment has been minimized.

Copy link
Member Author

dg commented May 20, 2016

@JanTvrdik I'll use your more explicit tests

@dg dg force-pushed the dg:autowire-alt branch from 6cbe9c5 to 61e4ba7 May 20, 2016
@dg dg merged commit 83b6a27 into nette:master May 20, 2016
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@dg dg deleted the dg:autowire-alt branch May 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.