Modify how nested resolutions get cached#10
Merged
stevegrunwell merged 2 commits intodevelopfrom Feb 23, 2022
Merged
Conversation
Previously, calling `Container::get()` would set `Container::$cacheResolutions = true`, run `Container::make()`, then restore `Container::$cacheResolutions` to its default value of `false`.
This worked well-enough for simple dependencies, but if the callable used to resolve a dependency used `get()` instead of `make()` then the resolution caching could be broken, resulting in warnings like the following:
> PHP Warning: Undefined array key "some-abstract" in /path/to/Container.php
This commit fixes the issue in two ways:
1. Replace the boolean `Container::$cacheResolutions` property with `Container::$resolutionCacheDepth`, an integer that gets incremented with each level of recursion
* If this value is greater than 0, `make()` will automatically cache everything it resolves
2. Explicitly set `Container::$resolved[$abstract]` upon calling `get()` instead of relying on `make()` to set it.
lkwdwrd
approved these changes
Feb 23, 2022
Contributor
There was a problem hiding this comment.
This looks fine, but moves us a little further down this weird global state tracking path. If this ends up causing more problems in the future, let me know and I feel we should refactor to make this more thread-safe, which should also make weird edge cases less of a problem. Still this approach is faster than what I loosely have in mind, and if it's doing its job, we're only in a single threaded environment so there's no reason to refactor.
src/Container.php
Outdated
| /** | ||
| * Whether or not resolutions should be cached. | ||
| * | ||
| * By default, this will be false but will be set to `true` when calling `get()`. |
Contributor
There was a problem hiding this comment.
Suggested change
| * By default, this will be false but will be set to `true` when calling `get()`. | |
| * By default, this will be `0` and will get incremented when calling `get()`. |
| } else { | ||
| throw new ContainerException(sprintf('Unhandled definition type (%s)', gettype($config[$abstract]))); | ||
| } | ||
| } catch (RecursiveDependencyException $e) { |
Contributor
There was a problem hiding this comment.
Ah, this is much better. Good update.
Contributor
Author
|
That's definitely a path I'd like to look into when we have some more time 👍 |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, calling
Container::get()would setContainer::$cacheResolutions = true, runContainer::make(), then restoreContainer::$cacheResolutionsto its default value offalse.This worked well-enough for simple dependencies, but if the callable used to resolve a dependency used
get()instead ofmake()then the resolution caching could be broken, resulting in warnings like the following:This PR fixes the issue in two ways:
Container::$cacheResolutionsproperty withContainer::$resolutionCacheDepth, an integer that gets incremented with each level of recursionmake()will automatically cache everything it resolvesContainer::$resolved[$abstract]upon callingget()instead of relying onmake()to set it.