rdlowrey / auryn

IoC Dependency Injector
MIT License
723 stars 65 forks source link

Sharing same class but different instances #121

Closed GreeKatrina closed 1 year ago

GreeKatrina commented 8 years ago

I was wondering what the result of the two situations below would be. Or what the desired result would be, if it's not that right now.

$auryn = new Auryn\Injector;

$auryn->share('Person');
$person = $auryn->make('Person');
$person->setName('John Snow'); // I enjoy the GOT references in the docs =)

$person = new Person;
$person->setName('Arya Stark');
$auryn->share($person);

$final_person = $auryn->make('Person');

Will $final_person be the John Snow instance, or will the Arya Stark instance override it?

$auryn = new Auryn\Injector;

$person = new Person();
$person->setName('John Snow');

$person2 = new Person();
$person2->setName('Arya Stark');

$auryn->share($person);
$auryn->share($person2);

$person3 = $auryn->make('Person');

Pretty much the same question as the first one.

My initial assumption is that after sharing the first instance of Person, any attempt to share a different instance of Person afterwards would result in either an error or the first instance overriding the second (The answer to both questions would be John Snow).

However, after looking at the Injector->shareInstance() method, it seems like the answer to both of the questions would be Arya Stark.

staabm commented 8 years ago

@GreeKatrina so what is the purpose of the issue? Do you think something is wrong?

vlakarados commented 8 years ago

Despite this is a proper question to ask, showing that you've done some research before asking would be better, like at least answering your first question by simply running it and getting the name $final_person->getName(), if no exception was thrown, which would also answer the second question. Instead of asking something that is so easy to answer yourself, you could contribute by asking if two instances of same class could be shared, e.g. by using a straight object share $injector->share($objOfClassMyClass); and an aliased interface $injector->share('\MyClassInterface').

GreeKatrina commented 8 years ago

Sorry I wasn't clear in my question. My co-worker asked me what the results of these would be, so I took her code and pasted it here.

I tested the situations as you suggested and they both resulted in the names being Arya Stark.

I was wondering if this is the expected result. So, it's really only an issue/bug if you guys think the result should not be Arya Stark for both of the situations above. If it's the output you guys would want, then feel free to close this.

kelunik commented 8 years ago

It's always the last one shared that's used. Do you think there's something wrong with that? What did you expect?

GreeKatrina commented 8 years ago

Opinionated Statement: If you run into the situations above you're likely either writing bad code (like globals) or accidentally instantiating a class twice instead of once (and removing one instantiation fixes it). So the result of both situations are for worst case scenarios and not very critical if you're using the DI correctly.

My initial expectation of the outcome was to not allow the first instance to be overriden and/or throw an error if an attempt to is made. But after some thought, I think using the last instantiation of the class (how it is now) is the correct outcome. If you want to instantiate the class a second time, it's probably for a reason, even if the reason doesn't follow best practices.

The main reasons I brought it up was because I couldn't find the answer in the docs and that possibly the outcome wasn't what YOU guys wanted (I think "the customer is always right" should never be used in the programming sphere haha).

kelunik commented 8 years ago

Actually I also thought about adding an error if a class is shared twice, because I think it'll be almost always an error. /cc @rdlowrey

GreeKatrina commented 8 years ago

I agree it would almost always be an error. I am using it for a small project and haven't found a situation yet where I would need to share the class twice. Just my two cents.

bwoebi commented 8 years ago

I remember having had discussions with Levi and Daniel about unsharing (which led to its removal in the more current API design https://gist.github.com/rdlowrey/569996f23a436964d595) … The generic point was about sharing something once and then it remains shared and immutable.

Everything else just may lead to two parts of code sharing different objects of a same class, leading to unexpected behavior.

I guess it just was an oversight to actually not check whether the share already exists. \cc @rdlowrey

morrisonlevi commented 8 years ago

I think my opinion is that things should not be shared twice. I think we've already had a ticket on this issue, haven't we? I think sharing something twice should be documented as undefined behavior or we should throw an exception or something; that's my $0.02, anyway.

bwoebi commented 8 years ago

I'd prefer being safe here and be explicit (Exception).

elazar commented 8 years ago

Related: #130.

djmattyg007 commented 8 years ago

There is absolutely a need for being able to have two instances of the same class. A perfect example is database read/write splitting. I would instantiate two copies of the same database adapter - one configured for reading, the other for writing. Creating two subclasses just to achieve this may not be feasible and certainly isn't desirable.

kelunik commented 8 years ago

@djmattyg007 Having two classes makes the intent clear and declares which class needs which dependency. Otherwise, how should Auryn know which one to inject?

kelunik commented 8 years ago

@djmattyg007 You could also use two interfaces instead.

staabm commented 8 years ago

as far as I read the readme one can also inject based on parameter-name, see https://github.com/rdlowrey/auryn#injection-definitions

Because the Car constructor parameter we needed to define was named $engine, our definition specified an engine key whose value was the name of the class (V8) that we want to inject.

djmattyg007 commented 8 years ago

Yes, you would specify which is which by parameter name, which is something Auryn already theoretically supports.

kelunik commented 7 years ago

I think I'll fix this and throw an exception on double-share. I think it was never designed to be allowed and is a bug.

Danack commented 1 year ago

I was wondering if this is the expected result.

It's an expected result. But as other results could also be expected, and no-one should ever need to debug what is happening inside an injector, sharing the same object multiple times has been prevented in 2d03a9efa2e6f5444fb9293fd01c28ea5f79b741