HP-2824: Fix Failing Psalm Static Analysis in php-billing Composer Package Build #111
Conversation
WalkthroughException classes were migrated to store domain data in exception context via HasContextInterface/HasContext; several getters/private fields were removed. Also: psalm.xml gained ensureOverrideAttribute, FormulaEngine::__clone lost its return type, and a PHPDoc generic was added to PriceTypeCollection. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-01-13T12:20:09.570ZApplied to files:
🧬 Code graph analysis (1)src/Exception/ChargeStealingException.php (1)
🪛 GitHub Check: PHP 8.3src/Exception/ChargeStealingException.php[failure] 30-30: PossiblyUnusedMethod 🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Exception/CannotReassignException.php (1)
22-25: Context-based exception wiring looks good; consider preserving old accessor for BCThe move to a context-based exception (extra interface + trait) looks consistent and doesn’t introduce obvious functional issues. If previous versions exposed a dedicated property/getter for this metadata (e.g.,
getField()), consider re‑adding a thin accessor that reads from the stored context to avoid breaking external consumers that may still call it.
VadymHrechukha
changed the title
…d ChargeOverlappingException::getCharge() method
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Exception/ChargeOverlappingException.php (1)
30-33: Consider preserving the charge reference in context.The refactoring removed the ability to retrieve the original
ChargeInterfaceobject (the previousgetCharge()method is gone). While storing serializable data in context is good for logging, consider whether consumers of this exception need access to the full charge object for error handling or recovery logic.If the charge object is needed by exception handlers, consider adding it to the context:
$self->addContext([ 'chargeTypeName' => $charge->getType()->getName(), 'chargeActionTime' => $charge->getAction()->getTime()->format(DATE_ATOM), + 'charge' => $charge, ]);Alternatively, if only specific charge data is needed, extract and add those specific fields to the context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
psalm.xml(1 hunks)src/Exception/ChargeOverlappingException.php(2 hunks)src/formula/FormulaEngine.php(1 hunks)src/product/Domain/Model/Price/PriceTypeCollection.php(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Psalm Static Analysis
src/Exception/ChargeOverlappingException.php
[error] 23-23: Psalm: PossiblyUnusedMethod - Cannot find any calls to method hiqdev\php\billing\Exception\ChargeOverlappingException::forCharge (see https://psalm.dev/087)
🔇 Additional comments (4)
psalm.xml (1)
17-17: LGTM - Valid Psalm configuration.The addition of
ensureOverrideAttribute="false"is appropriate for disabling enforcement of the#[\Override]attribute, which is necessary if the project is not yet using PHP 8.3 features or if there are legitimate cases where methods override parent/interface methods without the attribute.src/formula/FormulaEngine.php (1)
235-235: LGTM - Correct __clone signature.Removing the return type declaration from
__clone()is correct. PHP's magic__clonemethod should not declare a return type, and this change ensures compatibility with PHP's requirements for magic methods.src/Exception/ChargeOverlappingException.php (2)
23-36: Verify method usage in the codebase.Psalm reports
forCharge()as a possibly unused static factory method. Search the codebase for all calls toChargeOverlappingException::forCharge()to determine if this method is actively used or represents dead code.
13-14: Verify thathidev\exception\HasContextandhidev\exception\HasContextInterfaceare available in the hiqdev/hidev package.The hiqdev/hidev package exists on Packagist, but I could not locate the specific exception classes referenced in the imports through public documentation. Confirm that these classes are present in the package version declared in composer.json and that the dependency is correctly installed.
…hargeStealingException::getCharge() method
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Exception/ChargeStealingException.php(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-13T12:20:09.570Z
Learnt from: SilverFire
Repo: hiqdev/php-billing PR: 93
File: src/product/BillingRegistry.php:5-5
Timestamp: 2025-01-13T12:20:09.570Z
Learning: The `hiqdev/php-billing` repository should not depend on `advancedhosters/billing-registry`. Use internal interfaces and implementations instead.
Applied to files:
src/Exception/ChargeStealingException.php
🔇 Additional comments (2)
src/Exception/ChargeStealingException.php (2)
26-28: LGTM! Context pattern implemented correctly.The integration of
HasContextInterfaceandHasContexttrait is implemented correctly, and storing the charge viaaddContext()aligns with the context-based storage pattern adopted across other exception classes in this PR.Also applies to: 38-38
14-15: Verify the namespace for HasContext imports.The imports use
hidev\exception\(without 'q'), which differs from the package'shiqdev\php\billing\pattern. Confirm whether this is the correct namespace or if it's a typo that would cause class-not-found errors.
…e problems for code inspecting context by this key.
2 participants
Summary by CodeRabbit
Refactor
Breaking Changes
Chore
Style
✏️ Tip: You can customize this high-level summary in your review settings.