DI::get shared caching - a more comprehensive solution #13846
Merged
Conversation
This file contains 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
This comment was marked as abuse.
This comment was marked as abuse.
sergeyklay
suggested changes
Feb 21, 2019
Codecov Report
@@ Coverage Diff @@
## 4.0.x #13846 +/- ##
=========================================
+ Coverage 66.2% 66.2% +<.01%
=========================================
Files 451 451
Lines 89787 89802 +15
=========================================
+ Hits 59443 59457 +14
- Misses 30344 30345 +1
Continue to review full report at Codecov.
|
This comment was marked as abuse.
This comment was marked as abuse.
sergeyklay
approved these changes
Feb 23, 2019
CameronHall
approved these changes
Feb 24, 2019
This comment was marked as abuse.
This comment was marked as abuse.
|
There are no specific tests in the PR but the suite is passing so for now I will merge this despite the fact that there is no clear indication as to what this PR resolves. |
This comment was marked as abuse.
This comment was marked as abuse.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hello! @virgofx @niden @sergeyklay
This is designed to help
In raising this pull request, I confirm the following (please check boxes):
Last night I read through issue #13440 for improving the DI to deal with process forking. I realized that the
DI::getmethod could be improved to allow for actual shared caching so possible thatDI::getcould be used through Phalcon instead ofDI::getShared(in as many places as possible). Next up was theisFreshInstancehack around and that lead me to @virgofx 's unmerged PR to fix an architectural flaw in how Phalcon initialized some objects.I see this PR being a companion to @virgofx 's PR. Hopefully the entire "isFreshInstance" code can be deleted from the DI class as it is just a hack to fix a design flaw.
My goal with this PR is to allow a service that was set to shared to be treated the same if it was retrieved through
getandgetShared. So I added shared cache short-circuiting inget. In the initial commit there is a commented line for the desired behaviour but to not break things as they are right this moment I'm using thegetSharedmethod since it contains the fresh instance hacks.One final thing. This PR will change the behaviour of
DI::getso that retrieving cached shared instances will not fire the"di:beforeServiceResolve"and"di:afterServiceResolve"events.getSharedalready behaves this way so its really just treatinggetwith shared services the same asgetShared.