Skip to content

Introduce Package::build()#33

Merged
Chrico merged 12 commits intomasterfrom
feature/add-package-boot
May 26, 2023
Merged

Introduce Package::build()#33
Chrico merged 12 commits intomasterfrom
feature/add-package-boot

Conversation

@gmazzap
Copy link
Copy Markdown
Contributor

@gmazzap gmazzap commented May 23, 2023

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)

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 the ExecutableModule::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 any ExecutableModule::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 that
Package::container() was called before Package::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 but boot() was not.
The second was a missing status between initialized and ready.


Edit 2023-05-25

1) Package::build() no more accepts modules

The only accepted way of adding modules is now Package::addModule(). As before the edit, passing modules to Package::boot() works, but it is deprecated.

2) Failures in build() are now caught if debug is off

There was a discrepancy before the edit. When calling $package->boot() directly, any exception thrown inside build() would be caught (assuming debug is off), but doing $package->build()->boot(), the exception would bubble up.
To make the two calls equivalent, I wrapped build() in a try/catch block, handling the catch branch in the same way it was done for boot().
That is when I realized that it would fire the Package::ACTION_FAILED_BOOT action hook in both cases, which sounded wrong, so I introduced Package::ACTION_FAILED_BUILD when the failures came from build().
That means that effectively when the failure comes from Package::build(), both actions would be fires, passing two different exceptions.

3) Failures in addModule() and connect() are now caught if debug is off

I also realized that if anyone would add a module using the Package::ACTION_INIT hook, and that Package::addModule() would cause an exception, that would be caught, but if Package::addModule() is called directly on the package instance, it would not. Same for Package::connect().
So I wrapped those two methods in try/catch as well, similar to what I did for Package::build().

At that point, I realized that having a code like this:

$package->addModule($module)->build()->boot();

assuming debug is off, if addModule() throws, also build() and then again boot() would throw, and all the 3 exceptions would be caught.
In such a situation, I made sure that:

  • the Package::ACTION_FAILED_BUILD is fired once, passing the exception thrown by addModule()
  • the Package::ACTION_FAILED_BOOT is fired once, passing the exception thrown by boot(), but whose previous is the exception thrown by build() whose previous is the exception thrown by addModule().

If the exception is thrown by build() instead of addModule(), the scenario is very similar, but this time the Package::ACTION_FAILED_BUILD action hook passes the exception thrown by build() and Package::ACTION_FAILED_BOOT passes the exception thrown by boot() whose previous is the exception thrown by build().

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.md

For 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,

gmazzap added 2 commits May 23, 2023 19:31
The new method allows building a container out of the package without
running any `ExecutableModule::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 that
`Package::container()` was called before `Package::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 but `boot()` was not.
The second was a missing status between initialized and ready.

Documentation and tests were added.
@gmazzap gmazzap requested review from Biont, Chrico and dnaber-de May 23, 2023 17:55
@codecov
Copy link
Copy Markdown

codecov Bot commented May 23, 2023

Codecov Report

Patch coverage: 98.41% and project coverage change: -0.45 ⚠️

Comparison is base (eb14a9b) 96.86% compared to head (507b004) 96.41%.

❗ Current head 507b004 differs from pull request most recent head af7bf82. Consider uploading reports for the commit af7bf82 to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##             master      #33      +/-   ##
============================================
- Coverage     96.86%   96.41%   -0.45%     
- Complexity      177      207      +30     
============================================
  Files             9        9              
  Lines           510      642     +132     
============================================
+ Hits            494      619     +125     
- Misses           16       23       +7     
Flag Coverage Δ
unittests 96.41% <98.41%> (-0.45%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Package.php 97.16% <98.41%> (-0.35%) ⬇️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gmazzap gmazzap changed the title Feature/add package boot Introduce Package::boot() May 23, 2023
@gmazzap gmazzap changed the title Introduce Package::boot() Introduce Package::build() May 23, 2023
Copy link
Copy Markdown
Member

@Biont Biont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I welcome this change and left a few inline suggestions regarding the docs

Comment thread docs/Package.md Outdated
Comment thread docs/Package.md Outdated
Comment thread docs/Package.md Outdated
Comment thread docs/Package.md Outdated
Comment thread src/Package.php Outdated
@gmazzap
Copy link
Copy Markdown
Contributor Author

gmazzap commented May 23, 2023

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.

Comment thread src/Package.php Outdated
Comment thread docs/Package.md
gmazzap added 2 commits May 25, 2023 00:33
Props @Chrico

Also, make sure failures inside `build()` are caught the same way they
are in `boot()`, and introduce a "failed build" action hook as the
counterpart of the "failed boot" action hook.
Implement a uniform "failure flow", catching all the errors when debug
is off, and collecting them in a Throwable's "previous" hierarchy.

All the application flows are documented in a separate document.

Fix and add new tests for all the flows.
@gmazzap
Copy link
Copy Markdown
Contributor Author

gmazzap commented May 24, 2023

@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!

@gmazzap gmazzap requested review from Biont, Chrico and shvlv May 24, 2023 23:12
Signed-off-by: Christian Leucht <3417446+Chrico@users.noreply.github.com>
Comment thread src/Package.php Outdated
Copy link
Copy Markdown
Member

@Chrico Chrico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work @gmazzap 💪🏻 . Looks really nice! I've just 1 comment (which is not really a blocker).

esurov and others added 3 commits May 25, 2023 14:45
* Update for phpunit

This allows to update phpunit/phpunit with it's dependencies to 8 to 9 and correctly work with PHP8.*

* phpunit 8 & 9

To satisfy PHP7.2 requirement we need to have dual version of PHP.

phpunit-update
@Chrico Chrico removed the request for review from dnaber-de May 26, 2023 06:11
@Chrico Chrico removed the request for review from shvlv May 26, 2023 06:11
@Chrico Chrico merged commit 0bde4d1 into master May 26, 2023
@Chrico Chrico deleted the feature/add-package-boot branch May 26, 2023 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants