Conversation
|
|
||
| foreach ($this->config->mappers as $mapperClass) { | ||
| /** @var Mapper $mapper */ | ||
| $mappers[] = $this->container->get($mapperClass, context: $context); |
There was a problem hiding this comment.
Wouldn't this cache make it so that a mapper could potentially receive a dependency with a stale state?
There was a problem hiding this comment.
Can you give me a scenario for this?
There was a problem hiding this comment.
Imagine a value object registered as a singleton at the start of the request. The mappers are resolved and cached. The value object is changed in the container for any reason, the next time the mapper that uses it is called, it will have the previous value?
Might be niche though, but could be a pain to debug
There was a problem hiding this comment.
Sounds pretty nice indeed, but possible. Perhaps we could just add a refresh() method that'd essentially do:
public function refresh(): self
{
$this->mappers = $this->resolveMappers();
return $this;
}The above is a global refresh. We could also play with the idea of giving fresh mappers to the caller only:
public function refresh(): self
{
$clone = clone($this);
$clone->mappers = $clone->resolveMappers();
return $clone;
}There was a problem hiding this comment.
I'm not sure I completely understand the problem?
Wouldn't this cache make it so that a mapper could potentially receive a dependency with a stale state?
Mappers themselves aren't singletons (they can't be because they require that context variable), so whenever they are retrieved from the container, it's a fresh new object.
There was a problem hiding this comment.
It's more about dependencies than mappers being singletons or not.
Let's say you have this:
class SomeMapper implements Mapper {
public function __construct(private SomeSingletonService $service) {}
}- Request start,
ObjectFactoryis created, mappers are resolved and cached SomeSingletonServicegets re-registered for whatever reason mid-requestSomeMappergets called, but it still holds the oldSomeSingletonServiceinstance
It's very niche and antipattern, but a possibility.
There was a problem hiding this comment.
Gotcha. Yeah I don't think that we need to consider that edge case because, as you say, that's a clear antipattern. I can only imagine this happening during internal framework testing, and in that case there would be better solutions.
|
Gonna merge, we can further reiterate on it if needed. |
Closes #1834