laminas / laminas-servicemanager

Factory-Driven Dependency Injection Container
https://docs.laminas.dev/laminas-servicemanager/
BSD 3-Clause "New" or "Revised" License
154 stars 57 forks source link

Programmatically set shared false on a service after it has been called once when shared was true return the previously created service. #229

Closed e-belair closed 8 months ago

e-belair commented 8 months ago

Bug Report

Programmatically set shared false on a service after it has been called once when shared was true return the previously created service.

Q A
Version(s) latest

Summary

I know that is not often we need to share or not a service during the execution. I had to do that while testing a service with different configurations.

So I discovered here that the sm first check if the service is cached before checking if it's shared. This works fine if we don't programmatically change the shared state of the service, but it can have a bad behavior if we do.

How to reproduce

// Assume that MyService::class is shared by default
$service1 = $sm->get(MyService::class)
$sm->setShared(MyService::class, false)
$service2 = $sm->get(MyService::class)
// $service1 and $service2 are the same instance

Expected behavior

$service1 and $service2 should not be the same instance has we set shared to false

Ocramius commented 8 months ago

I'd say that $sm mutation after use is a bad idea overall.

I'd be more inclined to "freeze" the service manager after configuration (which would require a new major version anyway), rather than allowing this sort of edge case.

Ocramius commented 8 months ago

I'd like to close this as "Won't fix", but the correct resolution would be to have a test case that says "this is expected / normal"

e-belair commented 8 months ago

I suspected the answer but thought reporting the bug was important. However, I could resolve the problem by unsetting the service to force its creation.

$sm->setService(MyService::class, null)
$sm->get(MyService::class)
Ocramius commented 8 months ago

Thanks!

I'll close here: will use the issue as reference, but I think we shouldn't invest more time into it (for now)