Fix status check in PackageProxyContainer
#49
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
When status is STATUS_INITIALIZED or above, it is possible to access the package container. Right now, PackageProxyContainer only access the container after booting which is wrong and make usage of build() "dangerous".
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #49 +/- ##
============================================
- Coverage 98.93% 98.42% -0.52%
- Complexity 234 238 +4
============================================
Files 10 10
Lines 566 570 +4
============================================
+ Hits 560 561 +1
- Misses 6 9 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
gmazzap
changed the title
PackageProxyContainer
tfrommen
approved these changes
May 24, 2024
gmazzap
added a commit
that referenced
this pull request
May 27, 2024
And fail a couple of edge-cases bugs discovered via new tests
Chrico
approved these changes
Aug 26, 2024
gmazzap
added a commit
that referenced
this pull request
Aug 27, 2024
See #47 **To be merged after #49** - Added the generic, statically-named `ACTION_MODULARITY_INIT` action hook - Refactor hooks triggering, using a map between statuses ans actions, introducing two new statuses: `STATUS_INIT` and `SATUS_DONE`(which replaces the now deprecated `STATUS_BOOTED`) - Deprecate `STATUS_MODULES_ADDED` (`STATUS_BOOTING` was already an alias) - Refactor hook triggering for failed connection, moving it to a separate method. Behavior change: connecting an alreayd connected package still fires a failed action hook but does not throw anymore. - Do not use `PackageProxyContainer` when the package to connect is initialized, considering its container is already available - Allow for multiple consecutive calls to `Package::boot()` and `Package::build()` avoiding throwing unnecessary exceptions - Add extensive inline documentation to explain the different parts of the bootstrapping flow - Add a large amount of tests to cover both existing but untested scenarios as well as the new/updated behaviors - Rework the "Package" and "Application flow" documentation chapters to make them more easily consumable, better account for latest additions (not limited to the changes in this commit).
This was referenced Aug 29, 2024
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.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix.
What is the current behavior? (You can also link to an open issue here)
A connected package's services can not be accessed if the package is "built"
What is the new behavior (if this is a feature change)?
A connected package's services can be accessed if the package is "built".
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No.
Other information:
The implementation of
Package::connect()predates the implementation ofPackage::built().Before
Package::built()was introduced in 1.7.0 it was only safe to access a Package's container after having calledPackage::boot().Now, we can do
$package->build()->container()without issues.The implementation of
PackageProxyContainerwas not updated when we introducedPackage::built(), so we can still not access services from connected packages if they are not booted.This means using
built()early andboot()later, which is the reason why we introducedbuilt()in the first place is "dangerous" because if someone connects a built package and tries to access its services they will get an exception.I committed a test to
masterthat tests there the bug, and this test is merged back into this branch to prove this is now solved.