Closed gmazzap closed 1 year ago
Patch coverage: 98.41
% and project coverage change: -0.45
:warning:
Comparison is base (
eb14a9b
) 96.86% compared to head (507b004
) 96.41%.:exclamation: Current head 507b004 differs from pull request most recent head af7bf82. Consider uploading reports for the commit af7bf82 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
I welcome this change and left a few inline suggestions regarding the docs
Thanks, @Biont! I accepted all your suggestions in a single commit to not mess too much with the PR log.
@Chrico @Biont After Chris's feedback and after thinking more about this thing, I made a few changes, see the Edit 2023-05-25 section in the main post.
It would be great if you could review it again. Thanks!
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature. The target release would be the next minor.
What is the current behavior? (You can also link to an open issue here)
It is impossible to obtain a container instance without executing all the
ExecutableModule::run()
methods, making unit tests harder.What is the new behavior (if this is a feature change)?
A new
Package::build()
method is added, allowing consumers to obtain a container instance without executing any of theExecutableModule::run()
methods.Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No, but it introduces a deprecation (more info below).
Other information:
The new
Package::build()
method allows building a container out of the package without running anyExecutableModule::run()
, simplifying unit tests.Passing default modules to
Package::boot()
is now deprecated for a better separation of the "building" and "booting" steps, but it continues to work while emitting a deprecation notice.There's an edge case in which passing modules to
Package::boot()
causes an exception, but one of the conditions is thatPackage::container()
was called beforePackage::boot()
, which caused an exception before anyway, so the change is 100% backward compatible.Two new package statuses have been added:
Package::STATUS_MODULES_ADDED
Package::STATUS_READY
The first is necessary to distinguish the status after
build()
was called butboot()
was not. The second was a missing status between initialized and ready.Edit 2023-05-25
1)
Package::build()
no more accepts modulesThe only accepted way of adding modules is now
Package::addModule()
. As before the edit, passing modules toPackage::boot()
works, but it is deprecated.2) Failures in
build()
are now caught if debug is offThere was a discrepancy before the edit. When calling
$package->boot()
directly, any exception thrown insidebuild()
would be caught (assuming debug is off), but doing$package->build()->boot()
, the exception would bubble up. To make the two calls equivalent, I wrappedbuild()
in atry
/catch
block, handling thecatch
branch in the same way it was done forboot()
. That is when I realized that it would fire thePackage::ACTION_FAILED_BOOT
action hook in both cases, which sounded wrong, so I introducedPackage::ACTION_FAILED_BUILD
when the failures came frombuild()
. That means that effectively when the failure comes fromPackage::build()
, both actions would be fires, passing two different exceptions.3) Failures in
addModule()
andconnect()
are now caught if debug is offI also realized that if anyone would add a module using the
Package::ACTION_INIT
hook, and thatPackage::addModule()
would cause an exception, that would be caught, but ifPackage::addModule()
is called directly on the package instance, it would not. Same forPackage::connect()
. So I wrapped those two methods intry
/catch
as well, similar to what I did forPackage::build()
.At that point, I realized that having a code like this:
assuming debug is off, if
addModule()
throws, alsobuild()
and then againboot()
would throw, and all the 3 exceptions would be caught. In such a situation, I made sure that:Package::ACTION_FAILED_BUILD
is fired once, passing the exception thrown byaddModule()
Package::ACTION_FAILED_BOOT
is fired once, passing the exception thrown byboot()
, but whoseprevious
is the exception thrown bybuild()
whoseprevious
is the exception thrown byaddModule()
.If the exception is thrown by
build()
instead ofaddModule()
, the scenario is very similar, but this time thePackage::ACTION_FAILED_BUILD
action hook passes the exception thrown bybuild()
andPackage::ACTION_FAILED_BOOT
passes the exception thrown byboot()
whoseprevious
is the exception thrown bybuild()
.4) Application flow documentation
With all these changes to the flow, the documentation chapter "What happens when
Package::boot()
is called", which was part of the "Package" chapter, needed a rewrite and extension. So I decided to extract it to a separate file: https://github.com/inpsyde/modularity/blob/feature/add-package-boot/docs/Applicaton-flow.mdFor those interested in reviewing this PR, I suggest a read, as it answers many of the possible "why" that might arise from reading the code.
5) Updated and extended test
Unit tests were adjusted and a few were added to account for all the changes described above,