Conversation
5bea956 to
6075bad
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1638 +/- ##
============================================
+ Coverage 98.11% 99.00% +0.88%
- Complexity 960 962 +2
============================================
Files 197 197
Lines 2231 2214 -17
============================================
+ Hits 2189 2192 +3
+ Misses 42 22 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6075bad to
a73fae7
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the ISO-code-related validators to resolve Sokil ISO Codes database instances via ContainerRegistry, making the optional dependency behavior testable and enabling reuse of those database instances. It also adds targeted unit tests for missing optional components and introduces a PhpBench benchmark to track performance.
Changes:
- Add container-managed, optional bindings for Sokil ISO Codes database classes via
ContainerRegistry. - Update ISO-code-related validators (country/currency/language/subdivision/phone) to fetch databases from the container and throw
MissingComposerDependencyExceptionwhen unavailable. - Expand unit tests for “missing optional dependency” behavior and add an ISO-codes benchmark suite.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ContainerRegistry.php | Registers optional container entries for Sokil ISO Codes databases and adds optional() helper. |
| src/Exceptions/MissingClassException.php | Adds a PSR-11 NotFoundExceptionInterface implementation for missing optional classes. |
| src/Validators/CountryCode.php | Resolves Countries from the container instead of instantiating directly. |
| src/Validators/CurrencyCode.php | Resolves Currencies from the container instead of instantiating directly. |
| src/Validators/LanguageCode.php | Resolves Languages from the container instead of instantiating directly. |
| src/Validators/SubdivisionCode.php | Resolves Countries/Subdivisions from the container instead of instantiating directly. |
| src/Validators/Phone.php | Resolves Countries from the container when a country code is provided. |
| tests/unit/ContainerRegistryTest.php | Adds coverage for ContainerRegistry::optional() throwing on missing classes. |
| tests/unit/Validators/CountryCodeTest.php | Adds test for missing ISO Codes component via container override. |
| tests/unit/Validators/CurrencyCodeTest.php | Adds test for missing ISO Codes component via container override. |
| tests/unit/Validators/LanguageCodeTest.php | Adds test for missing ISO Codes component via container override. |
| tests/unit/Validators/SubdivisionCodeTest.php | Adds test for missing ISO Codes component via container override. |
| tests/unit/Validators/PhoneTest.php | Adds test for missing ISO Codes component via container override. |
| tests/benchmark/IsoCodesBench.php | Adds benchmark coverage for ISO-code validator construction/evaluation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
henriquemoody
left a comment
There was a problem hiding this comment.
Great change! You're on a row!
Let's just agree on the approach with the container registry, but other than that, no reason not to merge it.
db1ff30 to
f457713
Compare
The main focus of this change is to make those optional dependencies more testable. Unfortunately, some phpstan-ignores had to be included, since ::set is not a PsrContainer method. We're only using it on tests though, so it's fine. It targets our php-di container for testing purposes only. The real implementation only relies on ::get. This change also has the side effect of improving the performance of those validators by not instantiating their databases each time a iso validator is built, achieving massive improvements in those scenarios. A small benchmark with no assertions was added to track that improvement.
f457713 to
d0fdab5
Compare
The main focus of this change is to make those optional dependencies more testable.
Unfortunately, some phpstan-ignores had to be included, since ::set is not a PsrContainer method. We're only using it on tests though, so it's fine. It targets our php-di container for testing purposes only. The real implementation only relies on ::get.
This change also has the side effect of improving the performance of those validators by not instantiating their databases each time a iso validator is built, achieving massive improvements in those scenarios. A small benchmark with no assertions was added to track that improvement.
This brings the total coverage to almost 99%, addressing one of the key uncoverable paths that existed before.