auraphp / Aura.Di

Dependency Injection System
MIT License
349 stars 63 forks source link

Using a service no longer works #107

Closed silentworks closed 8 years ago

silentworks commented 8 years ago

The issue seem to stem from https://github.com/auraphp/Aura.Di/blob/3.0.0-beta2/src/Container.php#L504, now that the Container is locked after the first instance is created, this makes it not possible to use services.

The code on http://auraphp.com/packages/Aura.Di/services.html no longer works because at the point of set https://github.com/auraphp/Aura.Di/blob/3.0.0-beta2/src/Container.php#L229 the lock is checked and it would be true hence throwing an Exception.

pmjones commented 8 years ago

Ah yes! Example should be updated to use lazyNew(), not newInstance().

Even so, have you followed the auto-locking discussion on the mailing list? Interested in your feedback, esp. after this experience.

silentworks commented 8 years ago

Yeah I had to change everything to lazyNew and lazy, I didn't know there was a discussion about it, I don't really pay much attention to the mailing list to be honest, I am mostly on irc. Post the link to the discussion here please.

pmjones commented 8 years ago

Discussion after-the-fact not obvious, here you go: https://groups.google.com/forum/#!topic/auraphp/kOunjCihqe4

harikt commented 8 years ago

I guess I am the only person who is opposing the auto locking of container :) .

harikt commented 8 years ago

If I am correct, this post ( http://blog.shameerc.com/2015/09/slim-3-replacing-pimple-with-auradi ) by @shameerc will also will get into trouble.

pmjones commented 8 years ago

@shameerc Do you have any comments on this?

@silentworks Do we feel this is still a problem?

harikt commented 8 years ago

@pmjones as I mentioned earlier this will break in zend-expressive for they are doing something https://github.com/zendframework/zend-expressive-skeleton/blob/master/src/ExpressiveInstaller/Resources/config/container-aura-di.php#L16-L24 . You can try installing the expressive skelton that asks a few questions, but if Aura.Di is installed it installs beta2 which breaks :( .

pmjones commented 8 years ago

@harikt That code only uses lazies, which should be OK. Can you say where the breaks are happening?

harikt commented 8 years ago

@pmjones thanks. That opens my eye that I need to do new here https://github.com/zendframework/zend-expressive-skeleton/pull/14/files#diff-f7c87241f85b266da97aac6ff2b8ce30R21 and not newInstance . But there is a problem with I could not alter the behaviour later as I discussed in group. If I am the only one that have trouble with the locking I will not force you to do stop for me.

Thank you.

silentworks commented 8 years ago

This is good for me, I changed over to using lazy* for all things and no longer have a problem.

harikt commented 8 years ago

Actually I have a question, is container locking applied to automatic resolution ? I am wonder whether this will make it work or break.

pmjones commented 8 years ago

Great question @harikt. My guess is "no break" but I'll take a look.

shameerc commented 8 years ago

@pmjones @harikt Sorry for being to late to respond, I didn't get a chance to look at this yet. This will cause some issues while setting up dependencies in my code. I think I will have to manually create the monolog object here. Creating the doctrine dbal object also need to be re-written, but not sure what is the best way. It needs the configuration from settings, but the container will get locked if I call get(). I personally think it's a bit overhead for any projects as we will spend more time on thinking how to write the di config.

pmjones commented 8 years ago

@shameerc No hurry, no worry :-)

Meanwhile, I assert that the "right way" to use a DI container is not "config, create, config, create, config, create" but instead to configure everything in advance, and then create the object(s) that you need from the container.

I have taken the liberty of modifying your dependency-creation code. The major differences are:

Also, the Controller setters now use lazyGet() instead of get().

Does that seem like a reasonable path to follow, or is it at odds with your goals?

harikt commented 8 years ago

@pmjones @shameerc probably there is a bug in the call for $settings .

https://gist.github.com/pmjones/31c8c55bee4003d87eee#file-dependencies-php-L29

It seems the $settings is taken from the $di. So may be there also need to wrap it I guess.

Sorry that the message was send when I pressed enter to paste the link.

Thank you

pmjones commented 8 years ago

@harikt Which one?

shameerc commented 8 years ago

@pmjones Thanks ton for refactoring my code :) There were couple of minor things, but I have fixed it.

pmjones commented 8 years ago

@shameerc sure :-)

The real question is, do you think that's a reasonable/fair/not-too-hard way to work? Or does the container locking really get in the way?

harikt commented 8 years ago

@pmjones I did updated the comment, probably you would have missed.

I am looking at this

$settings = $di->get('settings');

on line https://gist.github.com/pmjones/31c8c55bee4003d87eee#file-dependencies-php-L39 . It seems probably @shameerc has also missed the same ? https://github.com/shameerc/slim-skeleton/blob/master/app/config/dependencies.php#L28 . Else I am unsure how the $settings variable comes there.

pmjones commented 8 years ago

@harikt Yeah, I'm just going with what @shameerc posted. :-)

pmjones commented 8 years ago

@shameerc Incidentally, if the settings service is an object and not an array, and it has a get($key) method, you can make a lazy-call to it like so: $di->lazyGetCall('settings', 'get', 'database'). Then you can avoid the whole settings:database service.

harikt commented 8 years ago

@pmjones one of my suggestion is people who are new to the di, will be having hard time to learn and understand this. Most of them start simple with using it as a service , but we can slowly convert them. Not on a sudden. So I feel this locking will cause some trouble.

Just my thoughts.

pmjones commented 8 years ago

@harikt /me nods

I tried two different approaches: (1) throwing an exception on get() or newInstance() when the container was not locked, and then (2) locking the container on the first call to get() or newInstance(). I ended up with the latter so that users would not have to explicitly call lock().

Now, while I still think that get() and newInstance() should not be available until after locking, I am open to the idea of throwing the exception instead of auto-locking.

Thoughts?

harikt commented 8 years ago

@pmjones I don't know can we keep a mode / flag that can alter the behavior ?

For eg :

throw exception who want to notify ?

do how di worked without locking ?

Something like turning on / off the auto resolution ?

shameerc commented 8 years ago

I think it's reasonable way to work if there are some examples provided. But like @harikt mentioned many developers (like me) will find it difficult to start with, as locking is something most of them are not familiar with. IMHO, it's ok to have locking as long as it is well documented and we are able to provide some examples how to tackle issues with locking. :)

pmjones commented 8 years ago

@harikt I'm so tired of flags to modify the behavior. :-/ I'd've rather removed auto-resolution entirely, but there is a contingent that love it, so I've separated it out as much as possible.

harikt commented 8 years ago

I leave this thread for the good. Probably there could be some sort of scripts that can generate di config from classes may help people to quickly do the same.

I did something earlier, (https://github.com/friendsofaura/FOA.DiConfig) . Not saying that is the best one, but just thinking that will help to quickly setup something even if there is no auto resolution.

Thank you and sorry for striking against this :) . ( Don't know the exact term, not a native english speaker ).

pmjones commented 8 years ago

Sorry, @harikt , I'm not trying to drive you out. :-(

harikt commented 8 years ago

I'm not trying to drive you out.

No, no, no. I was not saying that. As I expressed my opinion I don't think there is anything more I can add here for I don't know a way to propose to fix this issue without having trouble to both ends.

pmjones commented 8 years ago

Ah, yes, that's fair. Thanks for saying; and, as always, thanks for your thoughtful consideration and analysis. :-)