HP-2496/Added methods findBehaviorByClass, findPricesByTypeName and u…#103
HP-2496/Added methods findBehaviorByClass, findPricesByTypeName and u…#103SilverFire merged 3 commits intohiqdev:masterfrom ValeriyShnurovoy:HP-2496/Extend_TariffTypeDefinition_in_BillingRegistry_with_hasBehavior_and_getBehavior_methods
Conversation
…nit tests for them
WalkthroughNew lookup methods were introduced to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TariffTypeDefinition
participant PriceTypeDefinition
participant TariffTypeBehaviorRegistry
participant BehaviorInterface
Client->>TariffTypeDefinition: findPricesByTypeName(typeName)
TariffTypeDefinition->>TariffTypeDefinition: ensureNotLocked()
TariffTypeDefinition->>TariffTypeDefinition: matchesPriceType(price, typeName)
TariffTypeDefinition-->>Client: array|null
Client->>TariffTypeDefinition: findBehaviorByClass(class)
TariffTypeDefinition->>TariffTypeDefinition: ensureNotLocked()
TariffTypeDefinition->>BehaviorInterface: check class
TariffTypeDefinition-->>Client: BehaviorInterface|null
Client->>PriceTypeDefinition: findBehaviorByClass(class)
PriceTypeDefinition->>BehaviorInterface: check class
PriceTypeDefinition-->>Client: BehaviorInterface|null
Client->>TariffTypeBehaviorRegistry: findBehaviorByClass(class)
TariffTypeBehaviorRegistry->>BehaviorInterface: check instanceof class
TariffTypeBehaviorRegistry-->>Client: BehaviorInterface|null
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/product/TariffTypeDefinition.php(4 hunks)src/product/TariffTypeDefinitionInterface.php(2 hunks)src/product/behavior/HasBehaviorsInterface.php(1 hunks)src/product/price/PriceTypeDefinition.php(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: the `hiqdev/php-billing` repository should not depend on `advancedhosters/billing-registry`. use int...
Learnt from: SilverFire
PR: hiqdev/php-billing#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/product/price/PriceTypeDefinition.phpsrc/product/TariffTypeDefinitionInterface.phpsrc/product/TariffTypeDefinition.php
🔇 Additional comments (7)
src/product/behavior/HasBehaviorsInterface.php (1)
22-22: LGTM! Clean interface extension.The new method declaration follows consistent patterns and provides a clear contract for behavior lookup by class name.
src/product/TariffTypeDefinitionInterface.php (2)
8-8: LGTM! Import added for method signature.The import is necessary to support the method signature in the interface.
42-42: LGTM! Interface method declaration is well-defined.The method signature is clear and follows consistent patterns with appropriate nullable return type.
src/product/price/PriceTypeDefinition.php (2)
8-8: LGTM! Import added for return type.The import is necessary to support the BehaviorInterface return type in the new method.
198-207: LGTM! Clean implementation of behavior lookup.The implementation correctly iterates through the behavior collection and uses instanceof to find matching behaviors. The logic is consistent with the existing hasBehavior method pattern.
src/product/TariffTypeDefinition.php (2)
130-141: LGTM! Clean behavior lookup implementation.The implementation correctly iterates through behaviors and uses instanceof for type checking, consistent with similar methods in the codebase.
107-110: Confirm comma-delimited type names for price matchingThe
matchesPriceType()method insrc/product/TariffTypeDefinition.php(lines 107–110) does:return str_ends_with($price->type()->getName(), ",$typeName");This assumes that
$price->type()->getName()returns a comma-separated hierarchy (e.g."tariffType,priceType") so that checking for",$typeName"correctly identifies the intended leaf type.• Please verify that your
TypeDefinitionInterface::getName()implementation indeed concatenates parent and child names with commas.
• If it only returns the bare price-type name (or uses a different delimiter), update the matching logic (for example, compare equality or split on comma) to avoid false positives/negatives.
| public function findPricesByTypeName(string $typeName): ?array | ||
| { | ||
| $prices = null; | ||
| $this->ensureNotLocked(); | ||
|
|
||
| foreach ($this->prices as $price) { | ||
| if ($this->matchesPriceType($price, $typeName)) { | ||
| $prices[] = $price; | ||
| } | ||
| } | ||
| return $prices; | ||
| } |
There was a problem hiding this comment.
Fix potential null array append issue.
The variable $prices is initialized as null but then array elements are appended to it on line 101, which will cause a PHP error.
public function findPricesByTypeName(string $typeName): ?array
{
- $prices = null;
+ $prices = [];
$this->ensureNotLocked();
foreach ($this->prices as $price) {
if ($this->matchesPriceType($price, $typeName)) {
$prices[] = $price;
}
}
- return $prices;
+ return empty($prices) ? null : $prices;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function findPricesByTypeName(string $typeName): ?array | |
| { | |
| $prices = null; | |
| $this->ensureNotLocked(); | |
| foreach ($this->prices as $price) { | |
| if ($this->matchesPriceType($price, $typeName)) { | |
| $prices[] = $price; | |
| } | |
| } | |
| return $prices; | |
| } | |
| public function findPricesByTypeName(string $typeName): ?array | |
| { | |
| $prices = []; | |
| $this->ensureNotLocked(); | |
| foreach ($this->prices as $price) { | |
| if ($this->matchesPriceType($price, $typeName)) { | |
| $prices[] = $price; | |
| } | |
| } | |
| return empty($prices) ? null : $prices; | |
| } |
🤖 Prompt for AI Agents
In src/product/TariffTypeDefinition.php around lines 94 to 105, the variable
$prices is initialized as null but then used as an array when appending
elements, causing a PHP error. Initialize $prices as an empty array instead of
null before the loop to fix this issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/product/behavior/TariffTypeBehaviorRegistry.php (1)
63-72: LGTM! Consider refactoring to reduce code duplication.The implementation correctly implements the
HasBehaviorsInterfacecontract and follows the expected pattern. However, there's significant code duplication with the existinghasBehaviormethod.Consider refactoring to eliminate duplication:
public function hasBehavior(string $behaviorClassName): bool { - foreach ($this->behaviorCollection as $behavior) { - if ($behavior instanceof $behaviorClassName) { - return true; - } - } - - return false; + return $this->findBehaviorByClass($behaviorClassName) !== null; }This approach leverages the new method while maintaining the same functionality and reducing code duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/product/TariffTypeDefinition.php(4 hunks)src/product/behavior/TariffTypeBehaviorRegistry.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/TariffTypeDefinition.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/product/behavior/TariffTypeBehaviorRegistry.php (3)
src/product/behavior/HasBehaviorsInterface.php (1)
findBehaviorByClass(22-22)src/product/TariffTypeDefinition.php (1)
findBehaviorByClass(128-131)src/product/price/PriceTypeDefinition.php (1)
findBehaviorByClass(198-207)
3 participants
…nit tests for them
Summary by CodeRabbit