HP-1751 Create configurations for billing types #93
Conversation
WalkthroughThis pull request introduces numerous new classes, interfaces, and exceptions that expand the billing system’s functionality. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InvoiceDescBuilder as InvoiceDescriptionsBuilder
participant Registry as BillingRegistry
participant PriceType as PriceTypeDefinition
Client->>InvoiceDescBuilder: build()
InvoiceDescBuilder->>Registry: priceTypes()
Registry->>PriceType: documentRepresentation()
PriceType-->>InvoiceDescBuilder: description data
InvoiceDescBuilder-->>Client: return descriptions array
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 21
🧹 Outside diff range and nitpick comments (15)
src/product/ServerProduct.php (2)
1-6: Add class documentation.Please add PHPDoc documentation to describe the purpose and responsibility of this class within the billing system. This will help other developers understand how
ServerProductfits into the billing type configurations.<?php declare(strict_types=1); namespace hiqdev\php\billing\product; +/** + * Represents a server product in the billing system. + * This class handles billing configuration for server-related products. + */ class ServerProduct implements ProductInterface
5-11: Consider expanding the server product implementation.The current implementation seems minimal for a server product in a billing system. Consider adding:
- Properties to store server specifications (e.g., CPU, RAM, storage)
- Methods to handle pricing tiers based on specifications
- Integration with the billing type configurations mentioned in the PR title
This will make the class more useful for real-world billing scenarios.
src/product/InvoiceDescriptionsBuilder.php (2)
7-12: Consider adding validation in the constructor.While the dependency injection is good, consider validating that the registry has price types to fail fast if the registry is empty or invalid.
public function __construct(BillingRegistry $registry) { + if (empty($registry->priceTypes())) { + throw new \InvalidArgumentException('Registry must contain price types'); + } $this->registry = $registry; }
5-6: Consider defining interfaces for better abstraction.Since this is part of a billing system that other modules might need to extend:
- Consider extracting an interface (e.g.,
InvoiceDescriptionsBuilderInterface)- Consider making the class final if it's not meant to be extended
interface InvoiceDescriptionsBuilderInterface { public function build(): array; } final class InvoiceDescriptionsBuilder implements InvoiceDescriptionsBuilderInterface { // ... existing implementation }src/product/PriceTypesCollection.php (3)
1-6: Add class documentation.Please add a PHPDoc block to describe the class purpose, responsibilities, and usage.
<?php declare(strict_types=1); namespace hiqdev\php\billing\product; +/** + * Collection of price type definitions for a tariff. + * Manages the creation and storage of different price types (monthly, overuse) + * within the context of a tariff configuration. + */ class PriceTypesCollection
7-7: Enhance property type safety and documentation.The property type could be more specific to ensure type safety.
- private array $prices = []; + /** @var array<string, PriceTypeDefinition> */ + private array $prices = [];
5-29: Consider adding helper methods for price type management.The collection would benefit from additional methods to improve its functionality and maintainability.
Consider adding these methods:
getPriceType(string $type): ?PriceTypeDefinition- Retrieve a specific price typehasPriceType(string $type): bool- Check if a price type existsgetAllPriceTypes(): array- Get all defined price typesremovePriceType(string $type): void- Remove a price typeThis would make the collection more flexible and easier to work with in different contexts.
src/product/BillingRegistry.php (3)
5-9: Add PHPDoc documentation for class and properties.The class and its properties would benefit from comprehensive PHPDoc blocks to improve code maintainability and IDE support.
+/** + * Registry for managing tariff types and their associated price types. + * Once locked, the registry prevents further modifications to ensure data integrity. + */ class BillingRegistry { + /** + * @var TariffType[] Array of registered tariff types + */ private array $tariffTypes = []; + + /** + * @var bool Whether the registry is locked for modifications + */ private bool $locked = false;
19-22: Add method to check lock state.Consider adding a public method to check if the registry is locked, which would be useful for conditional logic in consuming code.
public function lock(): void { $this->locked = true; } + +/** + * @return bool Whether the registry is locked for modifications + */ +public function isLocked(): bool +{ + return $this->locked; +}
5-32: Consider architectural improvements for dependency injection and testing.As a central component of the billing system, consider these architectural improvements:
- Make the class implement an interface for better dependency injection and testing
- Consider adding a factory method for creating pre-configured instances
- Add methods to export the configuration for validation or persistence
Example interface:
interface BillingRegistryInterface { public function addTariffType(TariffType $tariffType): void; public function lock(): void; public function isLocked(): bool; public function priceTypes(): \Generator; public function export(): array; }src/product/PriceTypeDefinition.php (1)
5-48: Consider architectural improvementsA few architectural suggestions to enhance the robustness of this class:
- Consider making the class immutable by cloning in builder methods
- Consider extracting an interface to define the contract
- Consider adding a
build()method that returns aPriceTypeand keeping collection building separateThis would improve thread safety and separation of concerns.
src/product/TariffType.php (2)
39-42: Remove unnecessary methodThe
withBehaviorsmethod doesn't serve any purpose as it only returns$this. Consider removing it or implementing actual functionality.
5-56: Consider architectural improvementsSeveral architectural improvements could make the class more robust:
- Define a
TariffTypeInterfaceto make the contract explicit- Add getters for accessing the configured state
- Make the class immutable after finalization
- Add proper exception classes instead of using generic ones
Would you like me to help with implementing these architectural improvements?
tests/unit/product/TariffTypeTest.php (2)
21-22: Use ::class constant instead of string literalUsing string literals for class names makes the test brittle to refactoring. Consider using the ::class constant for better maintainability.
$tariffType = new TariffType('server'); -$tariffType->ofProduct('ServerProductClass'); +$tariffType->ofProduct(ServerProduct::class);
67-76: Improve helper method documentation and reusabilityThe helper method could benefit from:
- Complete PHPDoc with parameter and return types
- Moving to a shared test trait for reuse across test classes
/** * Helper function to access private properties for testing. + * + * @param object $object Object to inspect + * @param string $propertyName Name of the private property + * @return mixed Value of the private property */ private function getPrivateProperty($object, $propertyName)Consider creating a
TestHelperTraitwith this method to avoid duplication across test classes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/product/BehaviorInterface.php(1 hunks)src/product/BillingRegistry.php(1 hunks)src/product/InvoiceDescriptionsBuilder.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)src/product/PriceTypesCollection.php(1 hunks)src/product/ProductInterface.php(1 hunks)src/product/ServerProduct.php(1 hunks)src/product/TariffType.php(1 hunks)tests/unit/product/TariffTypeTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/product/BehaviorInterface.php
- src/product/ProductInterface.php
🔇 Additional comments (2)
src/product/InvoiceDescriptionsBuilder.php (1)
1-6: LGTM! Clean and well-structured class declaration.
The namespace is properly defined and the class name clearly indicates its responsibility.
tests/unit/product/TariffTypeTest.php (1)
11-17: LGTM! Well-structured initialization test
The test properly verifies both the name initialization and the prices collection type.
src/product/ServerProduct.php
Outdated
| public function name(): string | ||
| { | ||
| // TODO: Implement name() method. | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Implement the required name() method to fulfill the ProductInterface contract
The verification confirms that:
ServerProductimplementsProductInterfacename(): stringis a required interface method- No other implementations exist in the codebase to reference
- The current TODO implementation will cause a runtime error
The method must be properly implemented to return a string value that identifies the server product.
🔗 Analysis chain
Implement the required name() method.
The current TODO implementation will cause a runtime error as it doesn't return a string value as required by the return type declaration. This method is crucial for product identification in the billing system.
Please implement this method before merging. The implementation should return a meaningful name that identifies the server product in the billing context.
Let's verify if there are any existing implementations we can reference:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other implementations of the name() method in the codebase
ast-grep --pattern 'function name(): string { $$$$ }'
Length of output: 55
Script:
#!/bin/bash
# Let's try a broader search for name() method implementations
rg "function name\(\)" -A 3
# Also search for the interface definition to understand the contract
rg "interface.*Product" -A 10
Length of output: 576
| public function build(): array | ||
| { | ||
| $descriptions = []; | ||
| foreach ($this->registry->priceTypes() as $priceType) { | ||
| $descriptions[] = $priceType->representInvoice(); | ||
| } | ||
|
|
||
| return $descriptions; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and consider using array_map.
The build method could benefit from several improvements:
- Error handling for potential failures in representInvoice()
- Using array_map for a more functional approach
- PHPDoc explaining the return format
+/**
+ * Builds an array of invoice descriptions from registered price types.
+ *
+ * @return array<int, mixed> Array of invoice descriptions
+ * @throws \RuntimeException When failed to represent a price type
+ */
public function build(): array
{
- $descriptions = [];
- foreach ($this->registry->priceTypes() as $priceType) {
- $descriptions[] = $priceType->representInvoice();
+ try {
+ return array_map(
+ static fn($priceType) => $priceType->representInvoice(),
+ $this->registry->priceTypes()
+ );
+ } catch (\Throwable $e) {
+ throw new \RuntimeException(
+ 'Failed to build invoice descriptions: ' . $e->getMessage(),
+ 0,
+ $e
+ );
}
-
- return $descriptions;
}📝 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 build(): array | |
| { | |
| $descriptions = []; | |
| foreach ($this->registry->priceTypes() as $priceType) { | |
| $descriptions[] = $priceType->representInvoice(); | |
| } | |
| return $descriptions; | |
| } | |
| /** | |
| * Builds an array of invoice descriptions from registered price types. | |
| * | |
| * @return array<int, mixed> Array of invoice descriptions | |
| * @throws \RuntimeException When failed to represent a price type | |
| */ | |
| public function build(): array | |
| { | |
| try { | |
| return array_map( | |
| static fn($priceType) => $priceType->representInvoice(), | |
| $this->registry->priceTypes() | |
| ); | |
| } catch (\Throwable $e) { | |
| throw new \RuntimeException( | |
| 'Failed to build invoice descriptions: ' . $e->getMessage(), | |
| 0, | |
| $e | |
| ); | |
| } | |
| } |
| foreach ($this->registry->priceTypes() as $priceType) { | ||
| $descriptions[] = $priceType->representInvoice(); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Integration verification reveals potential issues
The codebase analysis shows a potential mismatch in the implementation. While InvoiceDescriptionsBuilder calls representInvoice() on price types, the actual implementation shows:
BillingRegistry::priceTypes()exists but returns a Generator- Price types are managed through
PriceTypesCollectionandPriceTypeDefinition PriceTypeDefinitionhasinvoiceRepresentation()method, but norepresentInvoice()method- No class or interface was found implementing the required
representInvoice()method
This suggests that either:
- The
representInvoice()method is missing from the implementation - There's a naming mismatch between the expected and actual method names
- The implementation might be using a different pattern than what's being called
🔗 Analysis chain
Verify integration with dependent components.
Let's ensure the required methods exist in the dependent classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if BillingRegistry and price types have required methods
# Check BillingRegistry for priceTypes method
ast-grep --pattern 'class BillingRegistry {
$$$
public function priceTypes() {
$$$
}
$$$
}'
# Check for classes implementing representInvoice method
ast-grep --pattern 'class $_ {
$$$
public function representInvoice() {
$$$
}
$$$
}'
Length of output: 204
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find these methods
# Search for BillingRegistry class and priceTypes method
echo "=== Searching for BillingRegistry and priceTypes ==="
rg "class BillingRegistry" -A 10
rg "function priceTypes"
# Search for representInvoice method
echo -e "\n=== Searching for representInvoice method ==="
rg "function representInvoice"
# Search for any interface that might define representInvoice
echo -e "\n=== Searching for interfaces that might define representInvoice ==="
rg "interface.*Price" -A 10
Length of output: 4025
Script:
#!/bin/bash
# Let's check for TariffType class and its relationship with representInvoice
# Search for TariffType class/interface
echo "=== Searching for TariffType definition ==="
rg "class TariffType|interface TariffType" -A 10
# Search for files that might contain representInvoice in price or tariff related files
echo -e "\n=== Searching for representInvoice in price/tariff related files ==="
fd -e php | grep -i "price\|tariff" | xargs rg "representInvoice"
# Let's also check if there's an interface that might define this method
echo -e "\n=== Searching for interfaces that might define invoice representation ==="
rg "interface.*Invoice" -A 10
Length of output: 1992
Script:
#!/bin/bash
# Let's check PriceTypesCollection and its relationship with invoice representation
# Search for PriceTypesCollection
echo "=== Searching for PriceTypesCollection ==="
rg "class PriceTypesCollection" -A 10
# Search for any invoice-related methods in price types
echo -e "\n=== Searching for invoice-related methods in price types ==="
rg -i "invoice.*function|function.*invoice" -A 3
# Let's also check if there are any invoice-related traits
echo -e "\n=== Searching for invoice-related traits ==="
rg "trait.*Invoice" -A 10
Length of output: 1407
src/product/PriceTypesCollection.php
Outdated
| public function end(): TariffType | ||
| { | ||
| // Return to the parent TariffType context | ||
| } |
There was a problem hiding this comment.
Critical: Implement the end() method.
The end() method is incomplete and will cause a fatal error when called.
The method needs proper implementation to return to the parent TariffType context. Based on the builder pattern suggested by the method name, it likely needs a reference to the parent TariffType:
class PriceTypesCollection
{
+ private TariffType $parent;
+
+ public function __construct(TariffType $parent)
+ {
+ $this->parent = $parent;
+ }
+
// ... other methods ...
public function end(): TariffType
{
- // Return to the parent TariffType context
+ return $this->parent;
}
}Committable suggestion skipped: line range outside the PR's diff.
src/product/PriceTypesCollection.php
Outdated
| public function monthly(string $type): PriceTypeDefinition | ||
| { | ||
| $priceType = new PriceTypeDefinition(); | ||
| $this->prices[$type] = $priceType; | ||
|
|
||
| return $priceType; | ||
| } | ||
|
|
||
| public function overuse(string $type): PriceTypeDefinition | ||
| { | ||
| $priceType = new PriceTypeDefinition(); | ||
| $this->prices[$type] = $priceType; | ||
|
|
||
| return $priceType; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate code and add validation.
The monthly() and overuse() methods contain identical implementation, which violates the DRY principle. Additionally, there's no validation of the input type or handling of duplicate entries.
Consider refactoring like this:
+ private function createPriceType(string $type): PriceTypeDefinition
+ {
+ if (empty($type)) {
+ throw new \InvalidArgumentException('Price type cannot be empty');
+ }
+
+ if (isset($this->prices[$type])) {
+ throw new \RuntimeException("Price type '$type' already exists");
+ }
+
+ $priceType = new PriceTypeDefinition();
+ $this->prices[$type] = $priceType;
+
+ return $priceType;
+ }
+
public function monthly(string $type): PriceTypeDefinition
{
- $priceType = new PriceTypeDefinition();
- $this->prices[$type] = $priceType;
-
- return $priceType;
+ return $this->createPriceType($type);
}
public function overuse(string $type): PriceTypeDefinition
{
- $priceType = new PriceTypeDefinition();
- $this->prices[$type] = $priceType;
-
- return $priceType;
+ return $this->createPriceType($type);
}📝 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 monthly(string $type): PriceTypeDefinition | |
| { | |
| $priceType = new PriceTypeDefinition(); | |
| $this->prices[$type] = $priceType; | |
| return $priceType; | |
| } | |
| public function overuse(string $type): PriceTypeDefinition | |
| { | |
| $priceType = new PriceTypeDefinition(); | |
| $this->prices[$type] = $priceType; | |
| return $priceType; | |
| } | |
| private function createPriceType(string $type): PriceTypeDefinition | |
| { | |
| if (empty($type)) { | |
| throw new \InvalidArgumentException('Price type cannot be empty'); | |
| } | |
| if (isset($this->prices[$type])) { | |
| throw new \RuntimeException("Price type '$type' already exists"); | |
| } | |
| $priceType = new PriceTypeDefinition(); | |
| $this->prices[$type] = $priceType; | |
| return $priceType; | |
| } | |
| public function monthly(string $type): PriceTypeDefinition | |
| { | |
| return $this->createPriceType($type); | |
| } | |
| public function overuse(string $type): PriceTypeDefinition | |
| { | |
| return $this->createPriceType($type); | |
| } |
src/product/TariffType.php
Outdated
| public function __construct(string $name) | ||
| { | ||
| $this->name = $name; | ||
| $this->prices = new PriceTypesCollection(); | ||
| } |
There was a problem hiding this comment.
Add input validation and handle required properties
The constructor should:
- Validate that name is not empty
- Either require productClass in constructor or mark it nullable
Apply this diff:
public function __construct(string $name)
{
+ if (empty(trim($name))) {
+ throw new \InvalidArgumentException('Tariff name cannot be empty');
+ }
$this->name = $name;
$this->prices = new PriceTypesCollection();
}Consider also adding productClass to constructor since it appears to be required:
- private string $productClass;
+ private ?string $productClass = null;📝 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 __construct(string $name) | |
| { | |
| $this->name = $name; | |
| $this->prices = new PriceTypesCollection(); | |
| } | |
| public function __construct(string $name) | |
| { | |
| if (empty(trim($name))) { | |
| throw new \InvalidArgumentException('Tariff name cannot be empty'); | |
| } | |
| $this->name = $name; | |
| $this->prices = new PriceTypesCollection(); | |
| } |
| use hiqdev\php\billing\product\PriceTypesCollection; | ||
| use hiqdev\php\billing\product\TariffType; | ||
| use PHPUnit\Framework\TestCase; |
There was a problem hiding this comment.
Add missing imports for behavior-related classes
The test uses OncePerMonthPlanChangeBehavior but it's not imported. Also, it would be beneficial to import BehaviorInterface for proper type hinting.
Add these imports:
use hiqdev\php\billing\product\PriceTypesCollection;
use hiqdev\php\billing\product\TariffType;
+use hiqdev\php\billing\product\BehaviorInterface;
+use hiqdev\php\billing\product\OncePerMonthPlanChangeBehavior;
use PHPUnit\Framework\TestCase;📝 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.
| use hiqdev\php\billing\product\PriceTypesCollection; | |
| use hiqdev\php\billing\product\TariffType; | |
| use PHPUnit\Framework\TestCase; | |
| use hiqdev\php\billing\product\PriceTypesCollection; | |
| use hiqdev\php\billing\product\TariffType; | |
| use hiqdev\php\billing\product\BehaviorInterface; | |
| use hiqdev\php\billing\product\OncePerMonthPlanChangeBehavior; | |
| use PHPUnit\Framework\TestCase; |
| $tariffType = new TariffType('server'); | ||
| $behavior = new OncePerMonthPlanChangeBehavior(); | ||
| $tariffType->attach($behavior); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints for better type safety
The behavior attachment test should verify that the behavior implements BehaviorInterface. Consider adding type hints and assertions.
$tariffType = new TariffType('server');
$behavior = new OncePerMonthPlanChangeBehavior();
+$this->assertInstanceOf(BehaviorInterface::class, $behavior);
$tariffType->attach($behavior);📝 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.
| $tariffType = new TariffType('server'); | |
| $behavior = new OncePerMonthPlanChangeBehavior(); | |
| $tariffType->attach($behavior); | |
| $tariffType = new TariffType('server'); | |
| $behavior = new OncePerMonthPlanChangeBehavior(); | |
| $this->assertInstanceOf(BehaviorInterface::class, $behavior); | |
| $tariffType->attach($behavior); |
| $priceType = $prices->monthly('support_time'); | ||
| $priceType->unit('hour')->description('Monthly fee for support time'); | ||
| $priceType->end(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add assertions for price type configuration
The test configures the price type but doesn't verify if the unit and description were set correctly.
$priceType = $prices->monthly('support_time');
$priceType->unit('hour')->description('Monthly fee for support time');
$priceType->end();
+
+$this->assertEquals('hour', $this->getPrivateProperty($priceType, 'unit'));
+$this->assertEquals('Monthly fee for support time', $this->getPrivateProperty($priceType, 'description'));📝 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.
| $priceType = $prices->monthly('support_time'); | |
| $priceType->unit('hour')->description('Monthly fee for support time'); | |
| $priceType->end(); | |
| $priceType = $prices->monthly('support_time'); | |
| $priceType->unit('hour')->description('Monthly fee for support time'); | |
| $priceType->end(); | |
| $this->assertEquals('hour', $this->getPrivateProperty($priceType, 'unit')); | |
| $this->assertEquals('Monthly fee for support time', $this->getPrivateProperty($priceType, 'description')); |
| public function testEndLocksTariffType(): void | ||
| { | ||
| $tariffType = new TariffType('server'); | ||
| $tariffType->end(); | ||
|
|
||
| // Assuming TariffType has a `locked` private property | ||
| $isLocked = $this->getPrivateProperty($tariffType, 'locked'); | ||
| $this->assertTrue($isLocked, 'TariffType should be locked after calling end().'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for locked state behavior
The test verifies that the tariff type is locked but doesn't test that operations after locking throw exceptions. Consider adding test cases for operations attempted after locking.
$tariffType = new TariffType('server');
$tariffType->end();
-// Assuming TariffType has a `locked` private property
$isLocked = $this->getPrivateProperty($tariffType, 'locked');
$this->assertTrue($isLocked, 'TariffType should be locked after calling end().');
+
+$this->expectException(\RuntimeException::class);
+$tariffType->ofProduct(ServerProduct::class);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/unit/product/InvoiceDescriptionsBuilderTest.php (4)
19-22: Consider extracting hardcoded values to constants or data providers.The test contains hardcoded values in closures (
$100,$50). This makes the test less maintainable and harder to update if these values need to change.Consider refactoring like this:
+ private const MONTHLY_SUPPORT_FEE = 100; + private const OVERUSE_SUPPORT_FEE = 50; - ->invoiceRepresentation(function () { - return 'Invoice for support_time (monthly): $100'; - }) + ->invoiceRepresentation(function () { + return sprintf('Invoice for support_time (monthly): $%d', self::MONTHLY_SUPPORT_FEE); + })Also applies to: 27-30
11-35: Consider breaking down the test setup.The test setup is quite complex with multiple configurations. Consider breaking it down into smaller, focused test methods or using data providers.
You could split this into separate test methods:
testMonthlyInvoiceDescription()testOveruseInvoiceDescription()
38-40: Add assertions for registry state.The test should verify that the registry is properly configured and locked.
Consider adding these assertions:
$billingRegistry->addTariffType($serverTariffType); $billingRegistry->lock(); +$this->assertTrue($billingRegistry->isLocked(), 'Registry should be locked'); +$this->assertCount(1, $billingRegistry->getTariffTypes(), 'Registry should contain exactly one tariff type');
47-60: Consider adding more robust assertions.While the current assertions verify the basic functionality, consider adding:
- Null/empty checks
- Structure validation instead of exact string matches
- Edge case handling
Example additions:
+$this->assertNotEmpty($invoiceDescriptions, 'Invoice descriptions should not be empty'); +$this->assertMatchesRegularExpression( + '/Invoice for support_time \(monthly\): \$\d+/', + $invoiceDescriptions[0], + 'Monthly invoice should match expected format' +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/unit/product/InvoiceDescriptionsBuilderTest.php(1 hunks)
🔇 Additional comments (2)
tests/unit/product/InvoiceDescriptionsBuilderTest.php (2)
1-8: LGTM! Class declaration follows best practices.
The class is properly set up with strict types, correct namespace, and proper TestCase extension.
14-14: Verify the external class reference.
The class \hiapi\legacy\lib\billing\price\suggester\device\ServerPricesSuggester is referenced but might not be available in the test environment.
| public function testInvoiceDescriptionsBuilderWithRealClasses() | ||
| { | ||
| // Create TariffType with real prices and behaviors | ||
| $serverTariffType = (new TariffType('server')) | ||
| ->ofProduct(ServerProduct::class) | ||
| ->setPricesSuggester(\hiapi\legacy\lib\billing\price\suggester\device\ServerPricesSuggester::class) | ||
| ->withPrices() | ||
| ->monthly('support_time') | ||
| ->unit('hour') | ||
| ->description('Monthly fee for support time') | ||
| ->quantityFormatter(MonthlyQuantityFormatter::class) | ||
| ->invoiceRepresentation(function () { | ||
| return 'Invoice for support_time (monthly): $100'; | ||
| }) | ||
| ->end() | ||
| ->overuse('support_time') | ||
| ->unit('hour') | ||
| ->description('Support time overuse') | ||
| ->quantityFormatter(HourBasedOveruseQuantityFormatter::class) | ||
| ->invoiceRepresentation(function () { | ||
| return 'Invoice for support_time (overuse): $50'; | ||
| }) | ||
| ->end() | ||
| ->end() // Returns control to TariffType | ||
| ->withBehaviors() | ||
| ->attach(new OncePerMonthPlanChangeBehavior()) | ||
| ->end(); | ||
|
|
||
| // Create BillingRegistry and add the TariffType | ||
| $billingRegistry = new BillingRegistry(); | ||
| $billingRegistry->addTariffType($serverTariffType); | ||
| $billingRegistry->lock(); | ||
|
|
||
| // Build invoice descriptions | ||
| $builder = new InvoiceDescriptionsBuilder($billingRegistry); | ||
| $invoiceDescriptions = $builder->build(); | ||
|
|
||
| // Verify the results | ||
| $this->assertIsArray($invoiceDescriptions, 'build() should return an array of invoice descriptions.'); | ||
| $this->assertCount(2, $invoiceDescriptions, 'There should be 2 invoice descriptions generated.'); | ||
|
|
||
| $this->assertSame( | ||
| 'Invoice for support_time (monthly): $100', | ||
| $invoiceDescriptions[0], | ||
| 'The first description should match the expected monthly invoice description.' | ||
| ); | ||
|
|
||
| $this->assertSame( | ||
| 'Invoice for support_time (overuse): $50', | ||
| $invoiceDescriptions[1], | ||
| 'The second description should match the expected overuse invoice description.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the implementation findings, I can now provide the final response:
Additional test scenarios needed for InvoiceDescriptionsBuilder
The current test coverage is insufficient. Based on the implementation, add test cases for:
- Attempting to build descriptions with an unlocked registry (verify proper handling)
- Empty registry (no tariff types added)
- Registry with multiple tariff types
- Registry throwing RuntimeException when attempting modifications after being locked
The implementation shows that BillingRegistry has important validation logic that should be covered in the tests, particularly around the locking mechanism and empty states.
🔗 Analysis chain
Verify test coverage for InvoiceDescriptionsBuilder.
The class only tests the happy path with valid inputs. Consider adding tests for:
- Invalid configurations
- Empty registry
- Unlocked registry
Let me gather more information about the test coverage by checking the actual implementation of InvoiceDescriptionsBuilder and its usage.
Let me check the actual implementation to understand what scenarios need testing.
Let me check the BillingRegistry implementation since it's a dependency of InvoiceDescriptionsBuilder.
Let me check the BillingRegistry implementation directly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other test files related to InvoiceDescriptionsBuilder
fd -e php -g "*InvoiceDescriptionsBuilder*Test.php"
# Check for test methods in this file
ast-grep --pattern 'function test[A-Za-z]*\(\)'
Length of output: 157
Script:
#!/bin/bash
# Find the implementation file
fd -e php InvoiceDescriptionsBuilder.php
# Check for usage of InvoiceDescriptionsBuilder in the codebase
rg "InvoiceDescriptionsBuilder" -l
# Look for the implementation to understand what scenarios need testing
ast-grep --pattern 'class InvoiceDescriptionsBuilder'
Length of output: 273
Script:
#!/bin/bash
# Check the implementation of InvoiceDescriptionsBuilder
cat src/product/InvoiceDescriptionsBuilder.php
# Look for any error handling or validation in the implementation
rg -A 5 "throw|assert" src/product/InvoiceDescriptionsBuilder.php
Length of output: 615
Script:
#!/bin/bash
# Find and check BillingRegistry implementation
fd -e php BillingRegistry.php
# Look for validation or error handling in BillingRegistry
rg -A 5 "throw|assert|Exception" --type php BillingRegistry
Length of output: 181
Script:
#!/bin/bash
# Check BillingRegistry implementation
cat src/product/BillingRegistry.php
# Look for any test files related to BillingRegistry
fd -e php -g "*BillingRegistry*Test.php"
Length of output: 805
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/product/PriceTypeDefinition.php (1)
43-47: Improve the end() method implementationThe current implementation has several architectural issues:
- It creates a new empty collection instead of returning to the parent collection
- No validation of required properties
- The state isn't properly locked after finalization
Consider these architectural improvements:
- Add a reference to the parent collection
- Validate all required properties before finalization
- Implement state locking to prevent modifications after
end()is called- Return the parent collection for continued builder chain
Example implementation structure:
class PriceTypeDefinition { private PriceTypesCollection $parent; private bool $isFinalized = false; public function __construct(PriceTypesCollection $parent) { $this->parent = $parent; } private function ensureNotFinalized(): void { if ($this->isFinalized) { throw new \RuntimeException('Cannot modify finalized price type definition'); } } public function end(): PriceTypesCollection { $this->validate(); $this->isFinalized = true; return $this->parent; } private function validate(): void { // Validate all required properties } }src/product/TariffType.php (1)
41-44: Implement withBehaviors methodThe method currently returns self without any implementation. Consider adding behavior validation or initialization logic.
tests/unit/product/InvoiceDescriptionsBuilderTest.php (1)
27-29: Simplify representation logicThe document representation setup is complex and might be better extracted into a separate helper method or fixture.
Consider creating a helper method to set up document representations to improve test readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/product/BillingRegistry.php(1 hunks)src/product/BillingRegistryInterface.php(1 hunks)src/product/DocumentRepresentationInterface.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)src/product/TariffType.php(1 hunks)tests/unit/product/InvoiceDescriptionsBuilderTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/product/DocumentRepresentationInterface.php
- src/product/BillingRegistryInterface.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/BillingRegistry.php
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/TariffType.php
[failure] 11-11: UndefinedClass
src/product/TariffType.php:11:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Product does not exist (see https://psalm.dev/019)
[failure] 23-23: UndefinedClass
src/product/TariffType.php:23:31: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Product does not exist (see https://psalm.dev/019)
🔇 Additional comments (11)
src/product/PriceTypeDefinition.php (3)
5-14: Add class-level documentation and property initialization
The class lacks PHPDoc documentation and property initialization, which could lead to undefined property access.
15-41: Add input validation and method documentation
The builder methods need input validation and PHPDoc documentation. The formatter and representation classes should be validated.
49-52: Clarify the purpose of documentRepresentation method
The documentRepresentation method appears to be a no-op as it simply returns $this without any state changes. If this is a placeholder for future implementation, it should be documented.
Let's check if this method is used in the codebase:
Consider adding documentation to clarify the method's purpose:
+ /**
+ * Placeholder for document representation configuration.
+ * TODO: Implement document representation logic
+ *
+ * @return self
+ */
public function documentRepresentation(): self
{
return $this;
}src/product/TariffType.php (7)
30-34:
Implement price suggester functionality
The method contains only a placeholder comment. This could lead to unexpected behavior.
Would you like me to help implement the price suggester validation and storage functionality?
46-51: 🛠️ Refactor suggestion
Add validation for behavior attachment
The method should validate behavior uniqueness and compatibility.
public function attach(BehaviorInterface $behavior): self
{
+ $behaviorClass = get_class($behavior);
+ foreach ($this->behaviors as $existingBehavior) {
+ if ($behaviorClass === get_class($existingBehavior)) {
+ throw new \InvalidArgumentException("Behavior of type '$behaviorClass' is already attached");
+ }
+ }
$this->behaviors[] = $behavior;
return $this;
}53-57:
Implement state validation and locking
The end method needs proper implementation for validation and state locking.
- public function end(): self
+ public function finalize(): self
{
- // Validate the TariffType and lock its state
+ if ($this->product === null) {
+ throw new \RuntimeException('Product must be set before finalizing');
+ }
+ if ($this->prices->isEmpty()) {
+ throw new \RuntimeException('At least one price type must be defined');
+ }
+ $this->locked = true;
return $this;
}15-15: 🛠️ Refactor suggestion
Add type hint for behaviors array
The behaviors array should have a proper type hint to ensure type safety.
- private array $behaviors = [];
+ /** @var BehaviorInterface[] */
+ private array $behaviors = [];Likely invalid or redundant comment.
17-21: 🛠️ Refactor suggestion
Add name validation in constructor
The constructor should validate that the name is not empty.
public function __construct(string $name)
{
+ if (empty(trim($name))) {
+ throw new \InvalidArgumentException('Tariff name cannot be empty');
+ }
$this->name = $name;
$this->prices = new PriceTypesCollection();
}Likely invalid or redundant comment.
36-39: 🛠️ Refactor suggestion
Add validation in withPrices method
The method should validate that the product is set before allowing price manipulation.
public function withPrices(): PriceTypesCollection
{
+ if ($this->product === null) {
+ throw new \RuntimeException('Product must be set before configuring prices');
+ }
return $this->prices;
}Likely invalid or redundant comment.
23-28: 🛠️ Refactor suggestion
Add product validation in ofProduct method
The method should validate that the product is not null.
public function ofProduct(Product $product): self
{
+ if ($product === null) {
+ throw new \InvalidArgumentException('Product cannot be null');
+ }
$this->product = $product;
return $this;
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 23-23: UndefinedClass
src/product/TariffType.php:23:31: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Product does not exist (see https://psalm.dev/019)
tests/unit/product/InvoiceDescriptionsBuilderTest.php (1)
12-68: 🛠️ Refactor suggestion
Enhance test coverage with additional scenarios
The current test only covers the happy path. Consider adding test cases for:
- Invalid configurations
- Empty registry
- Unlocked registry
- Registry with multiple tariff types
Would you like me to help generate additional test cases?
| // Create TariffType with real prices and behaviors | ||
| $serverTariffType = (new TariffType('server')) | ||
| ->ofProduct(ServerProduct::class) | ||
| ->setPricesSuggester(\hiapi\legacy\lib\billing\price\suggester\device\ServerPricesSuggester::class) |
There was a problem hiding this comment.
💡 Codebase verification
Legacy class ServerPricesSuggester appears to be missing
The class \hiapi\legacy\lib\billing\price\suggester\device\ServerPricesSuggester is referenced in the test but doesn't exist in the codebase. This could lead to test failures. You should either:
- Move the actual class implementation to the specified legacy path
- Update the test to use a mock or a different existing price suggester
- Update the namespace if the class has been moved to a new location
🔗 Analysis chain
Verify class existence in test
The test uses ServerPricesSuggester from a legacy path. Consider moving this to a more maintainable location or mocking it for tests.
Let me check if this class actually exists in the legacy path or if it's just referenced in the test.
Let me try one more search to find any references or implementations of this class in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the legacy suggester class exists
rg -l "ServerPricesSuggester"
Length of output: 83
Script:
#!/bin/bash
# Check for the actual class file in the legacy path
fd -e php ServerPricesSuggester
# Also search for any class definition
ast-grep --pattern 'class ServerPricesSuggester'
Length of output: 84
Script:
#!/bin/bash
# Search for any file containing the full namespace path
rg -l "hiapi.legacy.lib.billing.price.suggester.device.ServerPricesSuggester"
# Search with different path separators
rg -l "hiapi/legacy/lib/billing/price/suggester/device/ServerPricesSuggester"
# Get context around the usage in the test file
rg -C 5 "ServerPricesSuggester" tests/unit/product/InvoiceDescriptionsBuilderTest.php
Length of output: 885
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/product/PriceTypesCollection.php (1)
7-7: Initialize prices array with type hint.Add a type hint to the prices array for better type safety and code readability.
- private array $prices = []; + /** @var array<string, PriceTypeDefinition> */ + private array $prices = [];src/product/PriceTypeDefinition.php (1)
9-15: Initialize properties with default values.Properties should be initialized to prevent undefined property access.
- private string $unit; + private string $unit = ''; - private string $description; + private string $description = ''; - private string $quantityFormatter; + private string $quantityFormatter = ''; - private string $invoiceRepresentation; + private string $invoiceRepresentation = '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/product/PriceTypeDefinition.php(1 hunks)src/product/PriceTypesCollection.php(1 hunks)src/product/TariffType.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/TariffType.php
[failure] 11-11: UndefinedClass
src/product/TariffType.php:11:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Product does not exist (see https://psalm.dev/019)
[failure] 23-23: UndefinedClass
src/product/TariffType.php:23:31: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Product does not exist (see https://psalm.dev/019)
src/product/PriceTypeDefinition.php
[failure] 55-55: UndefinedClass
src/product/PriceTypeDefinition.php:55:47: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 57-57: UndefinedClass
src/product/PriceTypeDefinition.php:57:20: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 60-60: UndefinedClass
src/product/PriceTypeDefinition.php:60:34: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\measure\RcpTrafCollector does not exist (see https://psalm.dev/019)
🔇 Additional comments (4)
src/product/PriceTypesCollection.php (1)
21-24: LGTM!
The helper method is well-designed, encapsulating the creation logic and maintaining the parent-child relationship.
src/product/TariffType.php (2)
46-51: 🛠️ Refactor suggestion
Add validation for behavior attachment.
The method should validate behavior uniqueness and compatibility.
public function attach(BehaviorInterface $behavior): self
{
+ $behaviorClass = get_class($behavior);
+ foreach ($this->behaviors as $existingBehavior) {
+ if ($behaviorClass === get_class($existingBehavior)) {
+ throw new \InvalidArgumentException("Behavior of type '$behaviorClass' is already attached");
+ }
+ }
$this->behaviors[] = $behavior;
return $this;
}Likely invalid or redundant comment.
53-57:
Implement tariff validation and state locking.
The end method needs proper validation of required properties and state locking mechanism.
public function end(): self
{
- // Validate the TariffType and lock its state
+ if (!isset($this->product)) {
+ throw new \RuntimeException('Product must be set before finalizing');
+ }
+ if (empty($this->prices)) {
+ throw new \RuntimeException('At least one price type must be defined');
+ }
return $this;
}Likely invalid or redundant comment.
src/product/PriceTypeDefinition.php (1)
49-53:
Add validation in end method.
The end method should validate that all required properties are set before returning to parent.
public function end(): PriceTypesCollection
{
- // Validate the PriceType and lock its state
+ $requiredProps = ['unit', 'description', 'quantityFormatter', 'invoiceRepresentation'];
+ foreach ($requiredProps as $prop) {
+ if (empty($this->$prop)) {
+ throw new \RuntimeException("$prop is required but not set");
+ }
+ }
return $this->parent;
}Likely invalid or redundant comment.
src/product/PriceTypesCollection.php
Outdated
| public function monthly(string $type): PriceTypeDefinition | ||
| { | ||
| $priceType = $this->createPriceTypeDefinition(); | ||
| $this->prices[$type] = $priceType; | ||
|
|
||
| return $priceType; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for duplicate price types.
The monthly and overuse methods should validate that the price type doesn't already exist to prevent overwriting.
public function monthly(string $type): PriceTypeDefinition
{
+ if (isset($this->prices[$type])) {
+ throw new \InvalidArgumentException("Price type '$type' already exists");
+ }
$priceType = $this->createPriceTypeDefinition();
$this->prices[$type] = $priceType;
return $priceType;
}Apply the same validation to the overuse method.
Also applies to: 26-32
src/product/TariffType.php
Outdated
| public function setPricesSuggester(string $suggesterClass): self | ||
| { | ||
| // Validate or store the suggester class | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
Implement price suggester functionality.
The method contains only a placeholder comment. This could lead to unexpected behavior.
Would you like me to help implement the price suggester validation and storage functionality?
src/product/TariffType.php
Outdated
|
|
||
| namespace hiqdev\php\billing\product; | ||
|
|
||
| use hiqdev\billing\registry\product\Product; |
There was a problem hiding this comment.
💡 Codebase verification
The Product class import path is incorrect
The import statement is using hiqdev\billing\registry\product\Product, but based on the codebase analysis, the actual namespace for product-related classes is hiqdev\php\billing\product. The Product class itself is not found, but there is a ProductInterface and ServerProduct in this namespace.
- Change the import in
src/product/TariffType.phpfrom:to either:use hiqdev\billing\registry\product\Product;
oruse hiqdev\php\billing\product\ProductInterface;
depending on which one is needed in the context.use hiqdev\php\billing\product\ServerProduct;
🔗 Analysis chain
Fix undefined Product class import.
The static analysis indicates that the Product class doesn't exist in the specified namespace.
Also applies to: 11-11
Let me gather more information about the Product class and its namespace.
Let me try one more search to find any potential Product class definition in vendor directories or related repositories.
Let me check the actual namespace of the found Product-related files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Product class definition
rg -t php "class Product"
# Search for Product usage
rg -t php "use.*Product"
Length of output: 231
Script:
#!/bin/bash
# Search for the complete namespace path
fd . -e php -x grep -l "namespace.*billing\\registry\\product"
# Search for any references to Product class usage
rg -t php "Product::" -A 2
rg -t php "instanceof Product" -A 2
Length of output: 474
Script:
#!/bin/bash
# Search for composer.json to check dependencies
cat composer.json
# Search for any PHP files containing Product in their name
fd -e php -g '*Product*.php'
Length of output: 3040
Script:
#!/bin/bash
# Check the namespace declarations in the found files
cat src/product/ProductInterface.php
cat src/product/ServerProduct.php
Length of output: 423
src/product/PriceTypeDefinition.php
Outdated
| public function documentRepresentation(): InvoiceRepresentationCollection | ||
| { | ||
| return new InvoiceRepresentationCollection($this); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing required dependency for invoice and measure classes
The classes InvoiceRepresentationCollection and RcpTrafCollector are referenced from the hiqdev\billing\registry namespace, but there's no corresponding dependency in composer.json that would provide these classes. This appears to be a missing dependency issue rather than undefined classes.
- Add the required dependency that provides the
hiqdev\billing\registrynamespace to composer.json - The dependency is likely a separate package from the same vendor (hiqdev)
🔗 Analysis chain
Fix undefined classes and add method documentation.
The static analysis indicates that InvoiceRepresentationCollection and RcpTrafCollector classes don't exist in the specified namespaces.
Also applies to: 60-63
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the undefined classes
rg -t php "class (InvoiceRepresentationCollection|RcpTrafCollector)"
# Search for their usage
rg -t php "use.*(InvoiceRepresentationCollection|RcpTrafCollector)"
Length of output: 245
Script:
#!/bin/bash
# Check if the class exists in the vendor directory
fd -t f "InvoiceRepresentationCollection.php"
# Check for any references to the namespace
rg -t php "hiqdev\\\\billing\\\\registry\\\\invoice"
# Check for RcpTrafCollector references
rg -t php "RcpTrafCollector"
Length of output: 594
Script:
#!/bin/bash
# Check for the measure namespace and RcpTrafCollector in vendor or source
fd -t f "RcpTrafCollector.php"
# Look for other classes in the measure namespace
rg -t php "hiqdev\\\\billing\\\\registry\\\\measure"
# Check composer.json for relevant dependencies
cat composer.json
Length of output: 3159
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 55-55: UndefinedClass
src/product/PriceTypeDefinition.php:55:47: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 57-57: UndefinedClass
src/product/PriceTypeDefinition.php:57:20: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/product/PriceTypeDefinition.php (2)
11-17: Initialize properties with default values.Properties should be initialized to prevent undefined property access.
- private Unit $unit; + private ?Unit $unit = null; - private string $description; + private string $description = ''; - private string $quantityFormatter; + private string $quantityFormatter = '';🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 11-11: UndefinedClass
src/product/PriceTypeDefinition.php:11:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Unit does not exist (see https://psalm.dev/019)
[failure] 17-17: UndefinedClass
src/product/PriceTypeDefinition.php:17:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
45-50: Remove or implement commented code.The commented out
invoiceRepresentationmethod should either be implemented or removed.If the method is needed, implement it. Otherwise, remove the commented code to maintain cleanliness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/product/BillingRegistry.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)src/product/PriceTypesCollection.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/BillingRegistry.php
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/PriceTypeDefinition.php
[failure] 11-11: UndefinedClass
src/product/PriceTypeDefinition.php:11:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Unit does not exist (see https://psalm.dev/019)
[failure] 17-17: UndefinedClass
src/product/PriceTypeDefinition.php:17:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 19-19: UndefinedClass
src/product/PriceTypeDefinition.php:19:80: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Type does not exist (see https://psalm.dev/019)
[failure] 21-21: UndefinedClass
src/product/PriceTypeDefinition.php:21:44: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 24-24: UndefinedClass
src/product/PriceTypeDefinition.php:24:26: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Unit does not exist (see https://psalm.dev/019)
[failure] 58-58: UndefinedClass
src/product/PriceTypeDefinition.php:58:47: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 63-63: UndefinedClass
src/product/PriceTypeDefinition.php:63:34: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\measure\RcpTrafCollector does not exist (see https://psalm.dev/019)
[failure] 68-68: UndefinedClass
src/product/PriceTypeDefinition.php:68:29: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Type does not exist (see https://psalm.dev/019)
src/product/PriceTypesCollection.php
[failure] 15-15: UndefinedClass
src/product/PriceTypesCollection.php:15:29: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Type does not exist (see https://psalm.dev/019)
[failure] 23-23: UndefinedClass
src/product/PriceTypesCollection.php:23:48: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Type does not exist (see https://psalm.dev/019)
🔇 Additional comments (3)
src/product/PriceTypeDefinition.php (3)
5-7:
Add missing dependency for billing registry classes.
The static analysis indicates missing classes from the hiqdev\billing\registry namespace.
Add the required dependency to your composer.json:
{
"require": {
+ "hiqdev/billing-registry": "^1.0"
}
}Likely invalid or redundant comment.
52-56: 🛠️ Refactor suggestion
Add validation in end() method.
The end() method should validate that all required properties are set before returning.
public function end(): PriceTypesCollection
{
- // Validate the PriceType and lock its state
+ if ($this->unit === null) {
+ throw new \RuntimeException('Unit must be set');
+ }
+ if (empty($this->description)) {
+ throw new \RuntimeException('Description must be set');
+ }
+ if (empty($this->quantityFormatter)) {
+ throw new \RuntimeException('Quantity formatter must be set');
+ }
return $this->parent;
}Likely invalid or redundant comment.
63-66: Add implementation for measuredWith method.
The method currently returns without any implementation.
Would you like me to help implement this method based on the usage patterns in the codebase?
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 63-63: UndefinedClass
src/product/PriceTypeDefinition.php:63:34: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\measure\RcpTrafCollector does not exist (see https://psalm.dev/019)
src/product/PriceTypesCollection.php
Outdated
| public function monthly(Type $type): PriceTypeDefinition | ||
| { | ||
| $priceType = $this->createPriceTypeDefinition($type); | ||
| $this->prices[$type->toTypeName()] = $priceType; | ||
|
|
||
| return $priceType; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate code in monthly() and overuse() methods.
The methods contain identical implementation which violates the DRY principle.
Consider extracting the common logic:
+ private function addPriceType(Type $type): PriceTypeDefinition
+ {
+ if (isset($this->prices[$type->toTypeName()])) {
+ throw new \InvalidArgumentException("Price type '{$type->toTypeName()}' already exists");
+ }
+ $priceType = $this->createPriceTypeDefinition($type);
+ $this->prices[$type->toTypeName()] = $priceType;
+ return $priceType;
+ }
+
public function monthly(Type $type): PriceTypeDefinition
{
- $priceType = $this->createPriceTypeDefinition($type);
- $this->prices[$type->toTypeName()] = $priceType;
- return $priceType;
+ return $this->addPriceType($type);
}
public function overuse(Type $type): PriceTypeDefinition
{
- $priceType = $this->createPriceTypeDefinition($type);
- $this->prices[$type->toTypeName()] = $priceType;
- return $priceType;
+ return $this->addPriceType($type);
}Also applies to: 28-34
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 15-15: UndefinedClass
src/product/PriceTypesCollection.php:15:29: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Type does not exist (see https://psalm.dev/019)
src/product/PriceTypesCollection.php
Outdated
| public function end(): TariffType | ||
| { | ||
| return $this->parent; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation in end() method.
The end() method should validate that at least one price type has been added before returning to the parent context.
public function end(): TariffType
{
+ if (empty($this->prices)) {
+ throw new \RuntimeException('At least one price type must be defined');
+ }
return $this->parent;
}📝 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 end(): TariffType | |
| { | |
| return $this->parent; | |
| } | |
| public function end(): TariffType | |
| { | |
| if (empty($this->prices)) { | |
| throw new \RuntimeException('At least one price type must be defined'); | |
| } | |
| return $this->parent; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/product/PriceTypeDefinition.php (1)
45-50: Remove commented-out code.The commented-out
invoiceRepresentationmethod should be either implemented or removed. Commented-out code can lead to confusion and maintenance issues.-// public function invoiceRepresentation(string $representationClass): self -// { -// $this->invoiceRepresentation = $representationClass; -// -// return $this; -// }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/product/BillingRegistryInterface.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)src/product/PriceTypesCollection.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/BillingRegistryInterface.php
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/PriceTypeDefinition.php
[failure] 17-17: UndefinedClass
src/product/PriceTypeDefinition.php:17:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 19-19: UndefinedClass
src/product/PriceTypeDefinition.php:19:80: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
[failure] 21-21: UndefinedClass
src/product/PriceTypeDefinition.php:21:44: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 58-58: UndefinedClass
src/product/PriceTypeDefinition.php:58:47: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 63-63: UndefinedClass
src/product/PriceTypeDefinition.php:63:34: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\measure\RcpTrafCollector does not exist (see https://psalm.dev/019)
[failure] 68-68: UndefinedClass
src/product/PriceTypeDefinition.php:68:29: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
src/product/PriceTypesCollection.php
[failure] 15-15: UndefinedClass
src/product/PriceTypesCollection.php:15:29: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
[failure] 23-23: UndefinedClass
src/product/PriceTypesCollection.php:23:48: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
[failure] 28-28: UndefinedClass
src/product/PriceTypesCollection.php:28:29: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
🔇 Additional comments (6)
src/product/PriceTypesCollection.php (4)
1-13: LGTM! Well-structured class initialization.
The class follows good practices with strict typing, proper namespace organization, and constructor property promotion.
23-26: LGTM! Well-designed helper method.
The helper method follows good practices with proper scoping and single responsibility.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 23-23: UndefinedClass
src/product/PriceTypesCollection.php:23:48: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
15-21: 🛠️ Refactor suggestion
Refactor duplicate code and add validation.
The implementation contains duplicate code with the overuse method and lacks validation for duplicate price types.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 15-15: UndefinedClass
src/product/PriceTypesCollection.php:15:29: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
36-39: 🛠️ Refactor suggestion
Add validation in end() method.
The method should validate the collection state before returning to the parent context.
src/product/PriceTypeDefinition.php (2)
24-43: 🛠️ Refactor suggestion
Add input validation and method documentation.
The builder methods need input validation and proper documentation.
5-18:
Add missing dependencies to composer.json.
The following classes are referenced but appear to be missing from dependencies:
hiqdev\billing\registry\invoice\InvoiceRepresentationCollectionhiqdev\billing\registry\product\PriceTypehiqdev\billing\registry\measure\RcpTrafCollector
Add the required package that provides these classes to your composer.json:
{
"require": {
+ "hiqdev/billing-registry": "^1.0"
}
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 17-17: UndefinedClass
src/product/PriceTypeDefinition.php:17:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/product/PriceTypesCollection.php (2)
24-34: Optimize getAllPrices method.The nested loops can be replaced with array_merge and array_values for better performance.
private function getAllPrices(): array { - $allPrices = []; - foreach ($this->pricesGroupedByPriceType as $prices) { - foreach ($prices as $price) { - $allPrices[] = $price; - } - } - - return $allPrices; + return array_merge(...array_values($this->pricesGroupedByPriceType)); }
36-43: Refactor duplicate code in price type methods.The monthly, overuse, feature, domain, certificate, and discount methods share identical implementation patterns.
+ private function createAndAddPriceType(GType $gType, PriceType $type): PriceTypeDefinition + { + $priceType = $this->createPriceTypeDefinition($gType, $type); + $this->addPriceTypeDefinition($type, $priceType); + return $priceType; + } + public function monthly(PriceType $type): PriceTypeDefinition { - $priceType = $this->createPriceTypeDefinition(GType::monthly, $type); - $this->addPriceTypeDefinition($type, $priceType); - return $priceType; + return $this->createAndAddPriceType(GType::monthly, $type); }Apply similar changes to other price type methods.
Also applies to: 55-62, 69-76, 78-85, 87-94, 96-103
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/product/BillingRegistry.php(1 hunks)src/product/BillingRegistryInterface.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)src/product/PriceTypesCollection.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/BillingRegistryInterface.php
🔇 Additional comments (2)
src/product/BillingRegistry.php (1)
27-34: 🛠️ Refactor suggestion
Add error handling for tariff type iteration.
The priceTypes method should handle potential errors from withPrices() calls.
public function priceTypes(): \Generator
{
foreach ($this->tariffTypes as $tariffType) {
- foreach ($tariffType->withPrices() as $priceTypeDefinition) {
- yield $priceTypeDefinition;
+ try {
+ foreach ($tariffType->withPrices() as $priceTypeDefinition) {
+ yield $priceTypeDefinition;
+ }
+ } catch (\Throwable $e) {
+ throw new \RuntimeException(
+ "Failed to retrieve prices for tariff type",
+ 0,
+ $e
+ );
}
}
}Likely invalid or redundant comment.
src/product/PriceTypeDefinition.php (1)
49-53: 🛠️ Refactor suggestion
Add validation in end() method.
The end() method should validate that all required properties are set before returning.
public function end(): PriceTypesCollection
{
- // Validate the PriceType and lock its state
+ $requiredProps = ['unit', 'description', 'quantityFormatter'];
+ foreach ($requiredProps as $prop) {
+ if (!isset($this->$prop)) {
+ throw new \RuntimeException("$prop is required but not set");
+ }
+ }
return $this->parent;
}Likely invalid or redundant comment.
src/product/PriceTypesCollection.php
Outdated
| private function addPriceTypeDefinition(PriceType $type, PriceTypeDefinition $priceTypeDefinition): void | ||
| { | ||
| $this->pricesGroupedByPriceType[$type->name][] = $priceTypeDefinition; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation in addPriceTypeDefinition method.
The method should validate the price type name and prevent duplicate entries.
private function addPriceTypeDefinition(PriceType $type, PriceTypeDefinition $priceTypeDefinition): void
{
+ if (empty($type->name)) {
+ throw new \InvalidArgumentException('Price type name cannot be empty');
+ }
$this->pricesGroupedByPriceType[$type->name][] = $priceTypeDefinition;
}📝 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.
| private function addPriceTypeDefinition(PriceType $type, PriceTypeDefinition $priceTypeDefinition): void | |
| { | |
| $this->pricesGroupedByPriceType[$type->name][] = $priceTypeDefinition; | |
| } | |
| private function addPriceTypeDefinition(PriceType $type, PriceTypeDefinition $priceTypeDefinition): void | |
| { | |
| if (empty($type->name)) { | |
| throw new \InvalidArgumentException('Price type name cannot be empty'); | |
| } | |
| $this->pricesGroupedByPriceType[$type->name][] = $priceTypeDefinition; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/product/PriceTypeDefinition.php (3)
18-19: Potential uninitialized property usage.
Although PHP enforces type hints, consider initializing the private property $unit with a default or in the constructor to avoid the risk of referencing it before the setter is called.
48-53: Validate description input if needed.
If an empty description is meaningless in your context, consider throwing an exception or logging a warning.
60-66: Check if $formatterClass exists.
The TODO comment suggests verifying if $formatterClass exists. Implement it to prevent runtime errors from invalid classes.src/product/BillingRegistry.php (1)
31-38: Consider adding error handling or fallback logic.
If a tariff type's withPrices() call throws or returns non-iterable data, it may cause unexpected behavior. You could wrap the iteration in a try-catch to safeguard.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/product/BillingRegistry.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)src/product/PriceTypesCollection.php(1 hunks)src/quantity/QuantityFormatterInterface.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/PriceTypesCollection.php
🔇 Additional comments (3)
src/product/PriceTypeDefinition.php (2)
26-34: Consider adding validation for GType, PriceType, or parent correctness.
In situations where the constructor receives invalid objects (e.g., null or inconsistent references), you might want to enforce stronger checks or gracefully handle those errors, preventing runtime exceptions later.
89-92: Implementation is incomplete.
The measuredWith method simply returns $this. Ensure that hooking RcpTrafCollector or other measure logic is actually performed if intended.
src/quantity/QuantityFormatterInterface.php (1)
1-28: All methods appear well-defined.
The interface methods are concise and logical, covering textual formatting, DB storage format, and client-friendly quantity values. Great job!
src/product/PriceTypeDefinition.php
Outdated
| public function unit(string $unit): self | ||
| { | ||
| $this->unit = Unit::create($unit); | ||
|
|
||
| return $this; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the unit.
If $unit can be empty or invalid, a runtime error might occur in Unit::create(). Add input validation to ensure $unit is not empty and is recognized by Unit::create().
src/product/PriceTypeDefinition.php
Outdated
| public function end(): PriceTypesCollection | ||
| { | ||
| // Validate the PriceType and lock its state | ||
| return $this->parent; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add final validation to the end() method.
It might be beneficial to validate that required properties (e.g., $unit, $description, and $quantityFormatterDefinition) are all set and valid before returning to the parent.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/product/PriceTypeDefinition.php (2)
16-17: Add class-level documentation for clarity.
This class defines a core entity for building price types but lacks a PHPDoc block describing its purpose, usage, and examples.
63-69: Address the TODO for the formatter class check.
Currently, there is a TODO comment but no actual validation. Ensure the class exists and implements the required interface.Would you like me to generate a quick fix for this check?
src/product/BillingRegistry.php (1)
16-23: Validate duplicates in addTariffType.
If TariffType is identified by something like getId(), consider preventing duplicates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/product/BillingRegistry.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/PriceTypeDefinition.php
[failure] 22-22: UndefinedClass
src/product/PriceTypeDefinition.php:22:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\formatter\QuantityFormatterDefinition does not exist (see https://psalm.dev/019)
[failure] 24-24: UndefinedClass
src/product/PriceTypeDefinition.php:24:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 26-26: UndefinedClass
src/product/PriceTypeDefinition.php:26:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\BehaviorCollection does not exist (see https://psalm.dev/019)
[failure] 31-31: UndefinedClass
src/product/PriceTypeDefinition.php:31:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\formatter\QuantityFormatterFactory does not exist (see https://psalm.dev/019)
[failure] 33-33: UndefinedClass
src/product/PriceTypeDefinition.php:33:40: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 34-34: UndefinedClass
src/product/PriceTypeDefinition.php:34:41: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\BehaviorCollection does not exist (see https://psalm.dev/019)
[failure] 60-60: UndefinedDocblockClass
src/product/PriceTypeDefinition.php:60:15: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\unit\FractionUnit does not exist (see https://psalm.dev/200)
[failure] 72-72: UndefinedClass
src/product/PriceTypeDefinition.php:72:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
src/product/BillingRegistry.php
[failure] 41-41: UndefinedDocblockClass
src/product/BillingRegistry.php:41:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
[failure] 59-59: UndefinedClass
src/product/BillingRegistry.php:59:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
🔇 Additional comments (3)
src/product/PriceTypeDefinition.php (3)
18-24: Initialize properties to avoid undefined usage.
Properties such as $unit, $description, etc., can be initialized with default values (e.g., empty strings) to prevent undefined usage.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 22-22: UndefinedClass
src/product/PriceTypeDefinition.php:22:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\formatter\QuantityFormatterDefinition does not exist (see https://psalm.dev/019)
[failure] 24-24: UndefinedClass
src/product/PriceTypeDefinition.php:24:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
44-49: Add input validation for unit.
If $unit can be empty or invalid, a runtime error might occur in Unit::create($unit). Validate that $unit is not empty and recognized by Unit::create.
81-85: Add final validation to end() method.
Before returning $this->parent, verify that you've set required properties (unit, description, and quantity formatter) to valid values.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/product/TariffType.php (1)
5-5:⚠️ Potential issueIncorrect namespace import for Product
It appears that the class "Product" doesn't exist in the "hiqdev\billing\registry\product" namespace, leading to a potential autoloading or class-not-found error. Consider referencing the correct Product class or interface in "hiqdev\php\billing\product" (e.g., "ProductInterface" or "ServerProduct").Apply this diff for a potential fix (switching to ProductInterface as an example):
-use hiqdev\billing\registry\product\Product; +use hiqdev\php\billing\product\ProductInterface as Product;
🧹 Nitpick comments (4)
src/product/TariffType.php (1)
33-37: Add implementation or remove placeholder
The "setPricesSuggester" method currently contains only a comment, leaving the purpose unfulfilled. You may want to either implement the logic (e.g., store or validate the suggester class and tie it into pricing logic) or remove it if it's not required.src/product/ConsumptionDefinition.php (1)
19-22: Empty groups() method
The "groups()" method is currently a placeholder. If you plan to implement grouping logic for consumption columns, consider adding relevant parameters, logic, or documentation. Otherwise, remove the method to avoid confusion.src/product/ConsumptionColumn.php (2)
11-16: Encapsulate logic behind meaningful method naming
The method "convertible()" sets "$isConvertible" to true. Though this is functional, you might consider using clearer naming (e.g., "markAsConvertible()") or adding inline documentation to clarify the context and ramifications.
23-28: Extend consistent naming
Similarly, "overMax()" sets "$isOverMax" to true. In combination with "convertible()", both methods could benefit from more descriptive names or docblocks to govern usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/product/ConsumptionColumn.php(1 hunks)src/product/ConsumptionDefinition.php(1 hunks)src/product/TariffType.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/ConsumptionDefinition.php
[failure] 7-7: UndefinedClass
src/product/ConsumptionDefinition.php:7:13: UndefinedClass: Class, interface or enum named hiqdev\php\billing\product\ConsumptionColumnCollection does not exist (see https://psalm.dev/019)
[failure] 11-11: UndefinedClass
src/product/ConsumptionDefinition.php:11:39: UndefinedClass: Class, interface or enum named hiqdev\php\billing\product\ConsumptionColumnCollection does not exist (see https://psalm.dev/019)
[failure] 14-14: UndefinedClass
src/product/ConsumptionDefinition.php:14:32: UndefinedClass: Class, interface or enum named hiqdev\php\billing\product\ConsumptionColumnCollection does not exist (see https://psalm.dev/019)
🔇 Additional comments (1)
src/product/TariffType.php (1)
26-31: 🛠️ Refactor suggestion
Validate the product class
When a product is assigned (via the "ofProduct" method), consider validating that the given class or object is correct. At minimum, ensure that it indeed conforms to your business logic or an expected interface to prevent accidental misuse.
public function ofProduct(Product $product): self
{
+ // e.g., validate or check interface
+ // if (!$product instanceof ProductInterface) {
+ // throw new \InvalidArgumentException('Provided product must implement ProductInterface');
+ // }
$this->product = $product;
return $this;
}Likely invalid or redundant comment.
src/product/TariffType.php
Outdated
| public function end(): self | ||
| { | ||
| // Validate the TariffType and lock its state | ||
| return $this; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Finalize or lock TariffType upon "end"
The "end()" method includes a comment about validating the TariffType and locking its state, but lacks any actual implementation. Consider adding the necessary steps to prevent further modification after this point, or rename it to better describe its purpose (e.g., "finalize" or "build").
|
|
||
| class ConsumptionDefinition | ||
| { | ||
| private ConsumptionColumnCollection $columnCollection; |
There was a problem hiding this comment.
Undefined ConsumptionColumnCollection class
Static analysis reports that "ConsumptionColumnCollection" is not found in the "hiqdev\php\billing\product" namespace. Make sure you have created and properly namespaced this class or import it from the correct location to avoid fatal errors.
Would you like assistance in defining or locating "ConsumptionColumnCollection"?
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 7-7: UndefinedClass
src/product/ConsumptionDefinition.php:7:13: UndefinedClass: Class, interface or enum named hiqdev\php\billing\product\ConsumptionColumnCollection does not exist (see https://psalm.dev/019)
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/product/ConsumptionDefinition.php (1)
7-7:⚠️ Potential issueUndefined ConsumptionColumnCollection class
The ConsumptionColumnCollection class is not found in the current namespace.
Please ensure the class is properly imported:
+use hiqdev\php\billing\product\ConsumptionColumnCollection;
🧹 Nitpick comments (5)
src/product/ConsumptionDefinition.php (2)
5-9: Add class documentationPlease add PHPDoc to describe the class purpose and its role in the billing system. This will help other developers understand how this class fits into the overall architecture.
+/** + * ConsumptionDefinition manages the configuration of consumption-based billing + * through collections of columns and groups. + * + * @see ConsumptionColumnCollection + * @see ConsumptionGroupCollection + */ class ConsumptionDefinition
10-14: Consider validating parent TariffType in constructorThe constructor accepts a TariffType without any validation. Consider adding validation to ensure the parent is in a valid state.
public function __construct(private readonly TariffType $parent) { + if (!$parent->isValid()) { + throw new \InvalidArgumentException('Invalid TariffType provided'); + } $this->columnCollection = new ConsumptionColumnCollection($this); $this->groupCollection = new ConsumptionGroupCollection($this); }src/product/ConsumptionGroupCollection.php (1)
22-29: Consider adding validation in group() methodThe group() method creates and stores groups without any validation or limits.
public function group(): ConsumptionGroup { + if (count($this->groups) >= 100) { + throw new \RuntimeException('Maximum number of groups exceeded'); + } $group = new ConsumptionGroup($this); $this->groups[] = $group; return $group; }src/product/ConsumptionGroup.php (1)
25-30: Add validation in add() methodThe add() method accepts PriceType objects without any validation.
public function add(PriceType $priceType): self { + if ($priceType->isExpired()) { + throw new \InvalidArgumentException('Cannot add expired price type'); + } $this->group[] = $priceType; return $this; }🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 25-25: UndefinedClass
src/product/ConsumptionGroup.php:25:25: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)src/product/ConsumptionColumn.php (1)
9-18: Add property documentationConsider adding PHPDoc comments to explain the purpose and impact of
$isConvertibleand$isOverMaxflags.+ /** + * Indicates if the consumption can be converted to different units + */ private bool $isConvertible = false; + /** + * Indicates if the consumption exceeds maximum allowed value + */ private bool $isOverMax = false;🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 15-15: UndefinedClass
src/product/ConsumptionColumn.php:15:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/product/BillingRegistry.php(1 hunks)src/product/ConsumptionColumn.php(1 hunks)src/product/ConsumptionColumnCollection.php(1 hunks)src/product/ConsumptionDefinition.php(1 hunks)src/product/ConsumptionGroup.php(1 hunks)src/product/ConsumptionGroupCollection.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/PriceTypeDefinition.php
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/ConsumptionGroup.php
[failure] 7-7: MissingTemplateParam
src/product/ConsumptionGroup.php:7:35: MissingTemplateParam: hiqdev\php\billing\product\ConsumptionGroup has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
[failure] 18-18: UndefinedDocblockClass
src/product/ConsumptionGroup.php:18:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/200)
[failure] 25-25: UndefinedClass
src/product/ConsumptionGroup.php:25:25: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
src/product/ConsumptionColumn.php
[failure] 15-15: UndefinedClass
src/product/ConsumptionColumn.php:15:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
[failure] 44-44: UndefinedClass
src/product/ConsumptionColumn.php:44:34: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
src/product/ConsumptionColumnCollection.php
[failure] 7-7: MissingTemplateParam
src/product/ConsumptionColumnCollection.php:7:46: MissingTemplateParam: hiqdev\php\billing\product\ConsumptionColumnCollection has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
[failure] 24-24: UndefinedClass
src/product/ConsumptionColumnCollection.php:24:28: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
src/product/ConsumptionGroupCollection.php
[failure] 5-5: MissingTemplateParam
src/product/ConsumptionGroupCollection.php:5:45: MissingTemplateParam: hiqdev\php\billing\product\ConsumptionGroupCollection has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
src/product/BillingRegistry.php
[failure] 41-41: UndefinedDocblockClass
src/product/BillingRegistry.php:41:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
[failure] 59-59: UndefinedClass
src/product/BillingRegistry.php:59:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
🔇 Additional comments (10)
src/product/BillingRegistry.php (5)
16-23: Add validation for duplicate tariff types.
The current implementation allows duplicate tariff types to be added, which could lead to data integrity issues.
25-28: LGTM! The lock mechanism is simple and effective.
The one-way locking mechanism prevents modifications after initialization.
30-37: Add error handling and improve return type specificity.
The method should handle potential errors from withPrices() and provide a more specific return type hint.
43-55: Add validation for representation class existence.
The method should validate that the provided class exists and implements RepresentationInterface.
57-70: Enhance error context in createQuantityFormatter.
When quantity formatters are not found, you throw a generic exception. Consider adding details about which $type was not found to aid troubleshooting.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 59-59: UndefinedClass
src/product/BillingRegistry.php:59:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
src/product/ConsumptionColumnCollection.php (3)
9-14: LGTM!
Good use of constructor property promotion and proper type declarations.
16-22: LGTM!
Proper implementation of the IteratorAggregate interface with clear return type documentation.
24-36: Verify PriceType class dependency
The code references PriceType from hiqdev\billing\registry\product namespace, but static analysis indicates this class might not exist.
If the class doesn't exist, consider creating it or updating the import path.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 24-24: UndefinedClass
src/product/ConsumptionColumnCollection.php:24:28: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
src/product/ConsumptionColumn.php (2)
20-42: LGTM!
Good implementation of fluent interface pattern with clear state management methods.
44-52: LGTM!
Clean implementation of accessor methods. Note that the PriceType dependency verification was requested in the previous file's review.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 44-44: UndefinedClass
src/product/ConsumptionColumn.php:44:34: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\PriceType does not exist (see https://psalm.dev/019)
src/product/BillingRegistry.php
Outdated
| public function getConsumptionColumns(): \Generator | ||
| { | ||
| foreach ($this->tariffTypes as $tariffType) { | ||
| yield $tariffType->consumption()->columns(); | ||
| } | ||
| } | ||
|
|
||
| public function getConsumptionGroups(): \Generator | ||
| { | ||
| foreach ($this->tariffTypes as $tariffType) { | ||
| yield $tariffType->consumption()->groups(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for consumption methods.
Both methods assume that consumption() will always return a valid object. Consider adding null checks and error handling.
public function getConsumptionColumns(): \Generator
{
foreach ($this->tariffTypes as $tariffType) {
- yield $tariffType->consumption()->columns();
+ $consumption = $tariffType->consumption();
+ if ($consumption === null) {
+ throw new \RuntimeException(
+ sprintf("No consumption data available for tariff type '%s'", $tariffType->getId())
+ );
+ }
+ yield $consumption->columns();
}
}
public function getConsumptionGroups(): \Generator
{
foreach ($this->tariffTypes as $tariffType) {
- yield $tariffType->consumption()->groups();
+ $consumption = $tariffType->consumption();
+ if ($consumption === null) {
+ throw new \RuntimeException(
+ sprintf("No consumption data available for tariff type '%s'", $tariffType->getId())
+ );
+ }
+ yield $consumption->groups();
}
}📝 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 getConsumptionColumns(): \Generator | |
| { | |
| foreach ($this->tariffTypes as $tariffType) { | |
| yield $tariffType->consumption()->columns(); | |
| } | |
| } | |
| public function getConsumptionGroups(): \Generator | |
| { | |
| foreach ($this->tariffTypes as $tariffType) { | |
| yield $tariffType->consumption()->groups(); | |
| } | |
| } | |
| public function getConsumptionColumns(): \Generator | |
| { | |
| foreach ($this->tariffTypes as $tariffType) { | |
| $consumption = $tariffType->consumption(); | |
| if ($consumption === null) { | |
| throw new \RuntimeException( | |
| sprintf("No consumption data available for tariff type '%s'", $tariffType->getId()) | |
| ); | |
| } | |
| yield $consumption->columns(); | |
| } | |
| } | |
| public function getConsumptionGroups(): \Generator | |
| { | |
| foreach ($this->tariffTypes as $tariffType) { | |
| $consumption = $tariffType->consumption(); | |
| if ($consumption === null) { | |
| throw new \RuntimeException( | |
| sprintf("No consumption data available for tariff type '%s'", $tariffType->getId()) | |
| ); | |
| } | |
| yield $consumption->groups(); | |
| } | |
| } |
| class ConsumptionGroupCollection implements \IteratorAggregate | ||
| { |
There was a problem hiding this comment.
Fix IteratorAggregate implementation and docblock return type
There are two issues with the iterator implementation:
- Missing type parameters for IteratorAggregate
- Incorrect return type in docblock (ConsumptionColumn[] vs ConsumptionGroup[])
-class ConsumptionGroupCollection implements \IteratorAggregate
+class ConsumptionGroupCollection implements \IteratorAggregate<int, ConsumptionGroup>
{
/** @var ConsumptionGroup[] */
private array $groups = [];
/**
- * @return ConsumptionColumn[]
+ * @return \Traversable<int, ConsumptionGroup>
*/
public function getIterator(): \Traversable
{
return new \ArrayIterator($this->groups);
}Also applies to: 14-20
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 5-5: MissingTemplateParam
src/product/ConsumptionGroupCollection.php:5:45: MissingTemplateParam: hiqdev\php\billing\product\ConsumptionGroupCollection has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
src/product/ConsumptionGroup.php
Outdated
| class ConsumptionGroup implements \IteratorAggregate | ||
| { | ||
| private array $group; |
There was a problem hiding this comment.
Fix IteratorAggregate implementation and initialize group array
The class is missing type parameters for IteratorAggregate and the group array is not initialized.
-class ConsumptionGroup implements \IteratorAggregate
+class ConsumptionGroup implements \IteratorAggregate<int, PriceType>
{
- private array $group;
+ /** @var PriceType[] */
+ private array $group = [];📝 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.
| class ConsumptionGroup implements \IteratorAggregate | |
| { | |
| private array $group; | |
| class ConsumptionGroup implements \IteratorAggregate<int, PriceType> | |
| { | |
| /** @var PriceType[] */ | |
| private array $group = []; |
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 7-7: MissingTemplateParam
src/product/ConsumptionGroup.php:7:35: MissingTemplateParam: hiqdev\php\billing\product\ConsumptionGroup has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
src/product/ConsumptionGroup.php
Outdated
| use hiqdev\billing\registry\product\PriceType; | ||
|
|
There was a problem hiding this comment.
Fix import and use statement for PriceType
The PriceType class is not found in the current namespace.
use hiqdev\billing\registry\product\PriceType;
+use hiqdev\php\billing\product\PriceType;📝 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.
| use hiqdev\billing\registry\product\PriceType; | |
| use hiqdev\php\billing\product\PriceType; | |
| class ConsumptionColumnCollection implements \IteratorAggregate | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type parameter to IteratorAggregate interface
The class implements IteratorAggregate without specifying the type parameter. In PHP 8.3, it's recommended to explicitly declare the type:
-class ConsumptionColumnCollection implements \IteratorAggregate
+class ConsumptionColumnCollection implements \IteratorAggregate<ConsumptionColumn>Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 7-7: MissingTemplateParam
src/product/ConsumptionColumnCollection.php:7:46: MissingTemplateParam: hiqdev\php\billing\product\ConsumptionColumnCollection has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/product/BehaviorInterface.php (1)
5-8: Consider renaming the method to be more descriptiveThe method name
class()is ambiguous and could potentially conflict with PHP'sclasskeyword. Consider using a more descriptive name that clearly indicates the method's purpose, such asgetClassName()orgetBehaviorClass().interface BehaviorInterface { - public function class(): string; + public function getBehaviorClass(): string; }src/product/BillingRegistry.php (2)
57-70: Enhance error context in createQuantityFormatterThe error message should include details about which type was not found.
- throw new QuantityFormatterNotFoundException('Quantity formatter not found'); + throw new QuantityFormatterNotFoundException( + sprintf("Quantity formatter not found for type '%s'", $type) + );🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 59-59: UndefinedClass
src/product/BillingRegistry.php:59:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
86-95: Add class validation to behavior methodSimilar to getRepresentationsByType, validate the behavior class before processing.
public function behavior(string $behaviorClassWrapper): \Generator { + if (!class_exists($behaviorClassWrapper)) { + throw new \InvalidArgumentException("Behavior class '$behaviorClassWrapper' does not exist"); + } + if (!is_subclass_of($behaviorClassWrapper, BehaviorInterface::class)) { + throw new \InvalidArgumentException( + "Class '$behaviorClassWrapper' must implement BehaviorInterface" + ); + } foreach ($this->priceTypes() as $priceTypeDefinition) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/product/BehaviorInterface.php(1 hunks)src/product/BillingRegistry.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/BillingRegistry.php
[failure] 41-41: UndefinedDocblockClass
src/product/BillingRegistry.php:41:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
[failure] 59-59: UndefinedClass
src/product/BillingRegistry.php:59:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
🔇 Additional comments (4)
src/product/BillingRegistry.php (4)
16-23: Add validation for duplicate tariff types
The method should prevent duplicate tariff types to maintain data integrity.
30-37: 🛠️ Refactor suggestion
Add error handling to priceTypes method
The method should handle potential errors from withPrices() calls.
public function priceTypes(): \Generator
{
foreach ($this->tariffTypes as $tariffType) {
- foreach ($tariffType->withPrices() as $priceTypeDefinition) {
- yield $priceTypeDefinition;
+ try {
+ foreach ($tariffType->withPrices() as $priceTypeDefinition) {
+ yield $priceTypeDefinition;
+ }
+ } catch (\Throwable $e) {
+ throw new \RuntimeException(
+ "Failed to retrieve prices for tariff type: " . $tariffType->getId(),
+ 0,
+ $e
+ );
}
}
}Likely invalid or redundant comment.
72-84: 🛠️ Refactor suggestion
Add error handling for consumption methods
Both methods need null checks and error handling for the consumption calls.
public function getConsumptionColumns(): \Generator
{
foreach ($this->tariffTypes as $tariffType) {
- yield $tariffType->consumption()->columns();
+ $consumption = $tariffType->consumption();
+ if ($consumption === null) {
+ throw new \RuntimeException(
+ sprintf("No consumption data available for tariff type '%s'", $tariffType->getId())
+ );
+ }
+ yield $consumption->columns();
}
}Likely invalid or redundant comment.
43-55: 🛠️ Refactor suggestion
Add class validation to getRepresentationsByType method
The method should validate that the provided class exists and implements the correct interface.
public function getRepresentationsByType(string $representationClass): array
{
+ if (!class_exists($representationClass)) {
+ throw new \InvalidArgumentException("Class '$representationClass' does not exist");
+ }
+ if (!is_subclass_of($representationClass, DocumentRepresentationInterface::class)) {
+ throw new \InvalidArgumentException(
+ "Class '$representationClass' must implement DocumentRepresentationInterface"
+ );
+ }
$representations = [];Likely invalid or redundant comment.
src/product/BillingRegistry.php
Outdated
| use hiqdev\billing\registry\invoice\RepresentationInterface; | ||
| use hiqdev\billing\registry\quantity\formatter\QuantityFormatterNotFoundException; | ||
| use hiqdev\billing\registry\quantity\FractionQuantityData; | ||
| use hiqdev\php\billing\type\Type; |
There was a problem hiding this comment.
Update import paths to match the codebase structure
The import paths for several classes don't match the codebase structure:
-use hiqdev\billing\registry\invoice\RepresentationInterface;
-use hiqdev\billing\registry\quantity\formatter\QuantityFormatterNotFoundException;
-use hiqdev\billing\registry\quantity\FractionQuantityData;
+use hiqdev\php\billing\product\DocumentRepresentationInterface;
+use hiqdev\php\billing\product\formatter\QuantityFormatterNotFoundException;
+use hiqdev\php\billing\product\quantity\FractionQuantityData;
use hiqdev\php\billing\type\Type;📝 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.
| use hiqdev\billing\registry\invoice\RepresentationInterface; | |
| use hiqdev\billing\registry\quantity\formatter\QuantityFormatterNotFoundException; | |
| use hiqdev\billing\registry\quantity\FractionQuantityData; | |
| use hiqdev\php\billing\type\Type; | |
| use hiqdev\php\billing\product\DocumentRepresentationInterface; | |
| use hiqdev\php\billing\product\formatter\QuantityFormatterNotFoundException; | |
| use hiqdev\php\billing\product\quantity\FractionQuantityData; | |
| use hiqdev\php\billing\type\Type; |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/product/PriceTypeDefinition.php (2)
88-92:⚠️ Potential issueAdd validation in end() method
The end() method should validate that all required properties are set before returning.
public function end(): PriceTypesCollection { - // Validate the PriceType and lock its state + if ($this->unit === null) { + throw new \RuntimeException('Unit is required but not set'); + } + if (empty($this->description)) { + throw new \RuntimeException('Description is required but not set'); + } + if ($this->quantityFormatterDefinition === null) { + throw new \RuntimeException('Quantity formatter is required but not set'); + } return $this->parent; }
99-102:⚠️ Potential issueRemove or implement measuredWith method
The method appears to be a no-op, returning $this without any implementation.
Either implement the method's logic or remove it if it's not needed. If it's intended for future use, document it as such:
+ /** + * @todo Implement measurement logic + * This method is intended to configure measurement collection using RcpTrafCollector + * but is currently not implemented. + */ public function measuredWith(\hiqdev\billing\registry\measure\RcpTrafCollector $param): self { + throw new \RuntimeException('Method not implemented'); - return $this; }
🧹 Nitpick comments (3)
src/product/BehaviorInterface.php (1)
5-10: Enhance interface documentationWhile marker interfaces are valid design patterns, the documentation could be more descriptive about:
- The purpose and benefits of marking a class as a product behavior
- Examples of concrete behaviors that might implement this interface
- How the interface integrates with the billing system
/** - * Empty interface for mark product behavior + * Marker interface for product behaviors in the billing system. + * + * Classes implementing this interface represent specific billing behaviors + * that can be attached to products. Examples might include: + * - Recurring billing behavior + * - Usage-based billing behavior + * - Tiered pricing behavior + * + * These behaviors can be registered with products through the BehaviorCollection. */src/product/PriceTypeDefinition.php (2)
41-44: Document or remove empty hook methodThe
init()method is marked as a hook but lacks documentation explaining its purpose and when it should be overridden.+ /** + * Hook method called after constructor initialization. + * Override this method in child classes to perform additional initialization. + */ protected function init(): void { - // Hook }
124-130: Fix documentation language and typosThe PHPDoc contains typos and is written in a non-English language.
/** - * це параметер визначає агрегатну функцію яка застосовується для щоденно записаних ресурсів щоб визнизначти - * місячне споживання за яке потрібно пробілити клієнта + * This parameter defines the aggregate function that is applied to daily recorded resources + * to determine the monthly consumption for which the client needs to be billed. * * @param Aggregate $aggregate * @return self */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/product/BehaviorInterface.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)
🔇 Additional comments (2)
src/product/PriceTypeDefinition.php (2)
5-16:
Add missing dependency in composer.json
The class has multiple dependencies from the hiqdev\billing\registry namespace, but there's no corresponding package dependency declared.
Add the required dependency to composer.json:
{
"require": {
+ "hiqdev/billing-registry": "^1.0"
}
}Likely invalid or redundant comment.
19-29: 🛠️ Refactor suggestion
Initialize properties to prevent undefined property access
Properties should be initialized with default values or marked as nullable to prevent undefined property access.
- private UnitInterface $unit;
+ private ?UnitInterface $unit = null;
- private string $description;
+ private string $description = '';
- private QuantityFormatterDefinition $quantityFormatterDefinition;
+ private ?QuantityFormatterDefinition $quantityFormatterDefinition = null;
private InvoiceRepresentationCollection $invoiceCollection;
private BehaviorCollection $behaviorCollection;
- private Aggregate $aggregate;
+ private ?Aggregate $aggregate = null;Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/product/BillingRegistry.php (5)
5-8:⚠️ Potential issueUpdate import paths to match the codebase structure
The import paths for several classes don't match the project's namespace structure. They should be updated to use the
hiqdev\php\billing\productnamespace.-use hiqdev\billing\registry\behavior\BehaviorNotFoundException; -use hiqdev\billing\registry\invoice\RepresentationInterface; -use hiqdev\billing\registry\quantity\formatter\QuantityFormatterNotFoundException; -use hiqdev\billing\registry\quantity\FractionQuantityData; +use hiqdev\php\billing\product\behavior\BehaviorNotFoundException; +use hiqdev\php\billing\product\invoice\RepresentationInterface; +use hiqdev\php\billing\product\quantity\formatter\QuantityFormatterNotFoundException; +use hiqdev\php\billing\product\quantity\FractionQuantityData;
18-25: 🛠️ Refactor suggestionEnhance validation in addTariffType method
While the basic locked state validation is good, consider these improvements:
- Add validation for duplicate tariff types (as previously suggested)
- Add type validation to ensure TariffType class exists
public function addTariffType(TariffType $tariffType): void { if ($this->locked) { throw new \RuntimeException("BillingRegistry is locked and cannot be modified."); } + if (!class_exists(TariffType::class)) { + throw new \RuntimeException("TariffType class is not defined."); + } + + foreach ($this->tariffTypes as $existingTariff) { + if ($existingTariff->getId() === $tariffType->getId()) { + throw new \RuntimeException("Tariff type with ID '{$tariffType->getId()}' already exists."); + } + } + $this->tariffTypes[] = $tariffType; }
32-39: 🛠️ Refactor suggestionAdd error handling and improve documentation
The method should handle potential errors from
withPrices()and include proper return type documentation.+/** + * @return \Generator<PriceTypeDefinition> + * @throws \RuntimeException When price retrieval fails + */ public function priceTypes(): \Generator { foreach ($this->tariffTypes as $tariffType) { + try { foreach ($tariffType->withPrices() as $priceTypeDefinition) { yield $priceTypeDefinition; } + } catch (\Throwable $e) { + throw new \RuntimeException( + "Failed to retrieve prices for tariff type: " . $tariffType->getId(), + 0, + $e + ); + } } }
45-57: 🛠️ Refactor suggestionAdd validation for representation class
The method should validate the existence and interface implementation of the provided class.
public function getRepresentationsByType(string $representationClass): array { + if (!class_exists($representationClass)) { + throw new \InvalidArgumentException("Class '$representationClass' does not exist"); + } + if (!is_subclass_of($representationClass, RepresentationInterface::class)) { + throw new \InvalidArgumentException("Class '$representationClass' must implement RepresentationInterface"); + } + $representations = []; foreach ($this->priceTypes() as $priceTypeDefinition) { foreach ($priceTypeDefinition->documentRepresentation() as $representation) {
79-91: 🛠️ Refactor suggestionAdd error handling for consumption methods
Both methods need null checks and proper error handling.
+/** + * @return \Generator<ConsumptionColumn> + * @throws \RuntimeException When consumption data is not available + */ public function getConsumptionColumns(): \Generator { foreach ($this->tariffTypes as $tariffType) { - yield $tariffType->consumption()->columns(); + $consumption = $tariffType->consumption(); + if ($consumption === null) { + throw new \RuntimeException( + sprintf("No consumption data available for tariff type '%s'", $tariffType->getId()) + ); + } + yield $consumption->columns(); } } +/** + * @return \Generator<ConsumptionGroup> + * @throws \RuntimeException When consumption data is not available + */ public function getConsumptionGroups(): \Generator { foreach ($this->tariffTypes as $tariffType) { - yield $tariffType->consumption()->groups(); + $consumption = $tariffType->consumption(); + if ($consumption === null) { + throw new \RuntimeException( + sprintf("No consumption data available for tariff type '%s'", $tariffType->getId()) + ); + } + yield $consumption->groups(); } }
🧹 Nitpick comments (1)
src/product/BillingRegistry.php (1)
12-13: Add class-level documentation and consider extracting constantsThe class would benefit from comprehensive documentation and constant extraction for error messages.
+/** + * Registry for managing billing-related entities such as tariff types, price types, + * and their associated behaviors. + * + * This class implements the Registry pattern to maintain a collection of tariff types + * and provide access to their various aspects such as price types, consumption data, + * and behaviors. + * + * @implements BillingRegistryInterface + */ class BillingRegistry implements BillingRegistryInterface { + private const ERROR_REGISTRY_LOCKED = "BillingRegistry is locked and cannot be modified."; + private const ERROR_DUPLICATE_TARIFF = "Tariff type with ID '%s' already exists."; + private const ERROR_NO_CONSUMPTION = "No consumption data available for tariff type '%s'";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/product/BillingRegistry.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/BillingRegistry.php
[failure] 43-43: UndefinedDocblockClass
src/product/BillingRegistry.php:43:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
[failure] 61-61: UndefinedClass
src/product/BillingRegistry.php:61:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
[failure] 107-107: UndefinedClass
src/product/BillingRegistry.php:107:19: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\BehaviorNotFoundException does not exist (see https://psalm.dev/019)
src/product/BillingRegistry.php
Outdated
| public function behavior(string $type, string $behaviorClassWrapper): BehaviorInterface | ||
| { | ||
| $type = $this->convertStringTypeToType($type); | ||
|
|
||
| foreach ($this->priceTypes() as $priceTypeDefinition) { | ||
| if ($priceTypeDefinition->hasType($type)) { | ||
| foreach ($priceTypeDefinition->withBehaviors() as $behavior) { | ||
| if ($behavior instanceof $behaviorClassWrapper) { | ||
| return $behavior; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| throw new BehaviorNotFoundException('Behavior class not found'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve behavior method documentation and error handling
The method needs better documentation, input validation, and more informative error messages.
+/**
+ * Retrieves a behavior of specified type and class.
+ *
+ * @param string $type The type identifier
+ * @param string $behaviorClassWrapper The behavior class to match
+ * @return BehaviorInterface The matched behavior
+ * @throws BehaviorNotFoundException When no matching behavior is found
+ * @throws \InvalidArgumentException When input parameters are invalid
+ */
public function behavior(string $type, string $behaviorClassWrapper): BehaviorInterface
{
+ if (empty($type)) {
+ throw new \InvalidArgumentException('Type cannot be empty');
+ }
+ if (!class_exists($behaviorClassWrapper)) {
+ throw new \InvalidArgumentException("Behavior class '$behaviorClassWrapper' does not exist");
+ }
+ if (!is_subclass_of($behaviorClassWrapper, BehaviorInterface::class)) {
+ throw new \InvalidArgumentException("Class '$behaviorClassWrapper' must implement BehaviorInterface");
+ }
+
$type = $this->convertStringTypeToType($type);
foreach ($this->priceTypes() as $priceTypeDefinition) {
if ($priceTypeDefinition->hasType($type)) {
foreach ($priceTypeDefinition->withBehaviors() as $behavior) {
if ($behavior instanceof $behaviorClassWrapper) {
return $behavior;
}
}
}
}
- throw new BehaviorNotFoundException('Behavior class not found');
+ throw new BehaviorNotFoundException(
+ sprintf("Behavior of class '%s' not found for type '%s'", $behaviorClassWrapper, $type)
+ );
}📝 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 behavior(string $type, string $behaviorClassWrapper): BehaviorInterface | |
| { | |
| $type = $this->convertStringTypeToType($type); | |
| foreach ($this->priceTypes() as $priceTypeDefinition) { | |
| if ($priceTypeDefinition->hasType($type)) { | |
| foreach ($priceTypeDefinition->withBehaviors() as $behavior) { | |
| if ($behavior instanceof $behaviorClassWrapper) { | |
| return $behavior; | |
| } | |
| } | |
| } | |
| } | |
| throw new BehaviorNotFoundException('Behavior class not found'); | |
| } | |
| /** | |
| * Retrieves a behavior of specified type and class. | |
| * | |
| * @param string $type The type identifier | |
| * @param string $behaviorClassWrapper The behavior class to match | |
| * @return BehaviorInterface The matched behavior | |
| * @throws BehaviorNotFoundException When no matching behavior is found | |
| * @throws \InvalidArgumentException When input parameters are invalid | |
| */ | |
| public function behavior(string $type, string $behaviorClassWrapper): BehaviorInterface | |
| { | |
| if (empty($type)) { | |
| throw new \InvalidArgumentException('Type cannot be empty'); | |
| } | |
| if (!class_exists($behaviorClassWrapper)) { | |
| throw new \InvalidArgumentException("Behavior class '$behaviorClassWrapper' does not exist"); | |
| } | |
| if (!is_subclass_of($behaviorClassWrapper, BehaviorInterface::class)) { | |
| throw new \InvalidArgumentException("Class '$behaviorClassWrapper' must implement BehaviorInterface"); | |
| } | |
| $type = $this->convertStringTypeToType($type); | |
| foreach ($this->priceTypes() as $priceTypeDefinition) { | |
| if ($priceTypeDefinition->hasType($type)) { | |
| foreach ($priceTypeDefinition->withBehaviors() as $behavior) { | |
| if ($behavior instanceof $behaviorClassWrapper) { | |
| return $behavior; | |
| } | |
| } | |
| } | |
| } | |
| throw new BehaviorNotFoundException( | |
| sprintf("Behavior of class '%s' not found for type '%s'", $behaviorClassWrapper, $type) | |
| ); | |
| } |
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 107-107: UndefinedClass
src/product/BillingRegistry.php:107:19: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\BehaviorNotFoundException does not exist (see https://psalm.dev/019)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/product/BillingRegistry.php (2)
13-15: Add validation for tariff types arrayConsider adding runtime validation to ensure tariff type uniqueness and validity.
Add a method to validate tariff types:
+ /** + * @param TariffType $tariffType + * @throws \InvalidArgumentException If tariff type is invalid + */ + private function validateTariffType(TariffType $tariffType): void + { + foreach ($this->tariffTypes as $existingTariff) { + if ($existingTariff->getId() === $tariffType->getId()) { + throw new \InvalidArgumentException( + sprintf('Tariff type with ID "%s" already exists', $tariffType->getId()) + ); + } + } + }
73-76: Add input validation to convertStringTypeToType methodThe method should validate the input string before conversion.
Apply this diff to add validation:
private function convertStringTypeToType(string $type): TypeInterface { + if (empty($type)) { + throw new \InvalidArgumentException('Type string cannot be empty'); + } return Type::anyId($type); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/product/BillingRegistry.php(1 hunks)src/product/TariffType.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/TariffType.php
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/BillingRegistry.php
[failure] 42-42: UndefinedDocblockClass
src/product/BillingRegistry.php:42:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
[failure] 60-60: UndefinedClass
src/product/BillingRegistry.php:60:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
🔇 Additional comments (5)
src/product/BillingRegistry.php (5)
58-71: 🛠️ Refactor suggestion
Improve createQuantityFormatter method validation and error handling
The method needs input validation and more descriptive error messages.
Apply this diff to improve the method:
+ /**
+ * @throws QuantityFormatterNotFoundException
+ * @throws \InvalidArgumentException
+ */
public function createQuantityFormatter(
string $type,
FractionQuantityData $data,
): array {
+ if (empty($type)) {
+ throw new \InvalidArgumentException('Type cannot be empty');
+ }
+
$type = $this->convertStringTypeToType($type);
foreach ($this->priceTypes() as $priceTypeDefinition) {
if ($priceTypeDefinition->hasType($type)) {
return $priceTypeDefinition->createQuantityFormatter($data);
}
}
- throw new QuantityFormatterNotFoundException('Quantity formatter not found');
+ throw new QuantityFormatterNotFoundException(
+ sprintf('Quantity formatter not found for type "%s"', $type)
+ );
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 60-60: UndefinedClass
src/product/BillingRegistry.php:60:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
5-8:
Fix incorrect import paths
The import paths for several classes don't match the project structure. They should be updated to use the correct namespace.
Apply this diff to fix the import paths:
-use hiqdev\billing\registry\invoice\RepresentationInterface;
-use hiqdev\billing\registry\quantity\formatter\QuantityFormatterNotFoundException;
-use hiqdev\billing\registry\quantity\FractionQuantityData;
+use hiqdev\php\billing\invoice\RepresentationInterface;
+use hiqdev\php\billing\quantity\formatter\QuantityFormatterNotFoundException;
+use hiqdev\php\billing\quantity\FractionQuantityData;Likely invalid or redundant comment.
31-38: 🛠️ Refactor suggestion
Add error handling for withPrices calls
The method should handle potential errors when retrieving prices from tariff types.
Apply this diff to add error handling:
public function priceTypes(): \Generator
{
foreach ($this->tariffTypes as $tariffType) {
- foreach ($tariffType->withPrices() as $priceTypeDefinition) {
- yield $priceTypeDefinition;
+ try {
+ foreach ($tariffType->withPrices() as $priceTypeDefinition) {
+ yield $priceTypeDefinition;
+ }
+ } catch (\Throwable $e) {
+ throw new \RuntimeException(
+ sprintf('Failed to retrieve prices from tariff type "%s": %s',
+ $tariffType->getId(),
+ $e->getMessage()
+ ),
+ 0,
+ $e
+ );
}
}
}Likely invalid or redundant comment.
44-56: 🛠️ Refactor suggestion
Validate representation class parameter
The method should validate that the provided class exists and implements RepresentationInterface.
Apply this diff to add validation:
public function getRepresentationsByType(string $representationClass): array
{
+ if (!class_exists($representationClass)) {
+ throw new \InvalidArgumentException(
+ sprintf('Representation class "%s" does not exist', $representationClass)
+ );
+ }
+ if (!is_subclass_of($representationClass, RepresentationInterface::class)) {
+ throw new \InvalidArgumentException(
+ sprintf('Class "%s" must implement RepresentationInterface', $representationClass)
+ );
+ }
+
$representations = [];
foreach ($this->priceTypes() as $priceTypeDefinition) {Likely invalid or redundant comment.
17-24: 🛠️ Refactor suggestion
Enhance tariff type validation in addTariffType method
The method should validate the tariff type before adding it to the registry.
Apply this diff to improve validation:
public function addTariffType(TariffType $tariffType): void
{
if ($this->locked) {
throw new \RuntimeException("BillingRegistry is locked and cannot be modified.");
}
+ $this->validateTariffType($tariffType);
$this->tariffTypes[] = $tariffType;
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
src/product/PriceTypeDefinition.php (5)
5-12:⚠️ Potential issueAdd missing dependency for registry namespace
The class depends on multiple classes from the
hiqdev\billing\registrynamespace, but there's no corresponding dependency in composer.json.Add the required dependency to composer.json that provides the
hiqdev\billing\registrynamespace.
48-53:⚠️ Potential issueAdd input validation for the unit
The unit value should be validated before passing to Unit::create().
If $unit can be empty or invalid, a runtime error might occur in Unit::create(). Add input validation to ensure $unit is not empty and is recognized by Unit::create().
72-78:⚠️ Potential issueImplement class existence validation
The TODO comment indicates missing validation for the formatter class.
Add validation to ensure the formatter class exists and implements the correct interface.
90-94:⚠️ Potential issueAdd validation in end() method
The end() method should validate that all required properties are set before returning.
Add validation to ensure required properties (unit, description, quantityFormatterDefinition) are set before returning to the parent.
101-104:⚠️ Potential issueImplement the measuredWith logic
The method currently returns $this without implementing any logic.
Either implement the measurement logic or remove the method if it's not needed.
🧹 Nitpick comments (12)
src/product/PriceTypeDefinitionCollection.php (3)
10-16: Use explicit array typing for clarity
The private array property$pricesGroupedByPriceTypecould be more self-documenting if you annotate it with@var array<string, PriceTypeDefinition[]>, helping IDEs and static analyzers understand its structure./** * @var array<string, PriceTypeDefinition[]> */ private array $pricesGroupedByPriceType = [];
18-36: Consider implementing Countable
Your class implements\IteratorAggregateto allow iteration over the list of prices. If you also implement\Countable, you may conveniently provide the number of price definitions.
38-108: Reduce code duplication for similar methods
All methods (e.g.,monthly,overuse,domain, etc.) differ only byGType. Consider creating a single private helper that accepts aGTypeandPriceTypeand returns aPriceTypeDefinition, to streamline the code.-public function monthly(PriceType $type): PriceTypeDefinition -{ - $priceType = $this->createPriceTypeDefinition(GType::monthly, $type, $this->parent->tariffType()); - $this->addPriceTypeDefinition($type, $priceType); - return $priceType; -} ... +private function definePrice(GType $gType, PriceType $type): PriceTypeDefinition +{ + $priceType = $this->createPriceTypeDefinition($gType, $type, $this->parent->tariffType()); + $this->addPriceTypeDefinition($type, $priceType); + return $priceType; +} + +public function monthly(PriceType $type): PriceTypeDefinition +{ + return $this->definePrice(GType::monthly, $type); +}src/product/BehaviorCollection.php (2)
7-7: Annotate IteratorAggregate with a generic
Psalm or PhpStan may complain about missing generic params. Consider adding template annotations, e.g.IteratorAggregate<BehaviorInterface>, to satisfy stricter static analyzers.-class BehaviorCollection implements \IteratorAggregate +/** + * @implements \IteratorAggregate<int, BehaviorInterface> + */ +class BehaviorCollection implements \IteratorAggregate🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 7-7: MissingTemplateParam
src/product/BehaviorCollection.php:7:37: MissingTemplateParam: hiqdev\php\billing\product\BehaviorCollection has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
24-31: Validate BehaviorInterface implementations
Before attaching a behavior, consider checking if it indeed matches your expected interface shape (e.g., ensuring mandatory methods exist). This can help catch misconfigurations earlier.src/product/TariffTypeDefinition.php (2)
1-21: Document how $product is utilized
Although$productis stored, you may want to include brief doc comments about its role within the definition (e.g., whether it’s used for referencing metadata or external logic).
23-56: Extend method-level documentation
Some public methods likeend()andsetPricesSuggester()lack details on their purpose and expected usage. Adding doc comments can clarify behavior and make the API more discoverable.src/product/BillingRegistry.php (3)
17-24: Consider preventing duplicate tariff types
When adding a new tariff type, you may want to ensure uniqueness. If one with the same identity exists, throwing an exception guards against accidental overwrites.🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 17-17: MissingDependency
src/product/BillingRegistry.php:17:35: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
40-56: Short-circuit by returning early
IngetRepresentationsByType, consider short-circuiting once you find enough representations or if you anticipate large loops. This can improve performance in big registries.🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 42-42: UndefinedDocblockClass
src/product/BillingRegistry.php:42:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
58-71: Provide clearer error context
When throwingQuantityFormatterNotFoundException, consider including the requested$type, making debugging easier.-throw new QuantityFormatterNotFoundException('Quantity formatter not found'); +throw new QuantityFormatterNotFoundException( + sprintf("Quantity formatter not found for type '%s'", (string) $type) +);🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 60-60: UndefinedClass
src/product/BillingRegistry.php:60:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)src/product/PriceTypeDefinition.php (2)
43-46: Document or implement the init hookThe empty
init()method suggests it's a hook for initialization, but its purpose is not documented.Add PHPDoc explaining the purpose of this hook method and when it should be overridden:
+ /** + * Hook method for additional initialization in derived classes. + * Override this method to perform custom initialization logic. + */ protected function init(): void { // Hook }
127-129: Translate comments to EnglishThe PHPDoc comments are written in Ukrainian and should be translated to English for consistency.
Replace the Ukrainian comments with English translation:
- * це параметер визначає агрегатну функцію яка застосовується для щоденно записаних ресурсів щоб визнизначти - * місячне споживання за яке потрібно пробілити клієнта + * This parameter defines the aggregate function that is applied to daily recorded resources + * to determine the monthly consumption for which the client needs to be billed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/product/BehaviorCollection.php(1 hunks)src/product/BehaviorInterface.php(1 hunks)src/product/BillingRegistry.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)src/product/PriceTypeDefinitionCollection.php(1 hunks)src/product/TariffTypeDefinition.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/BehaviorInterface.php
[failure] 12-12: UndefinedClass
src/product/BehaviorInterface.php:12:35: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\domain\TariffType does not exist (see https://psalm.dev/019)
[failure] 14-14: UndefinedClass
src/product/BehaviorInterface.php:14:38: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\domain\TariffType does not exist (see https://psalm.dev/019)
src/product/BehaviorCollection.php
[failure] 7-7: MissingTemplateParam
src/product/BehaviorCollection.php:7:37: MissingTemplateParam: hiqdev\php\billing\product\BehaviorCollection has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
[failure] 12-12: UndefinedClass
src/product/BehaviorCollection.php:12:33: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\domain\TariffType does not exist (see https://psalm.dev/019)
src/product/BillingRegistry.php
[failure] 14-14: MissingDependency
src/product/BillingRegistry.php:14:5: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
[failure] 14-14: MissingDependency
src/product/BillingRegistry.php:14:13: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
[failure] 17-17: MissingDependency
src/product/BillingRegistry.php:17:35: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
[failure] 34-34: MissingDependency
src/product/BillingRegistry.php:34:22: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
[failure] 42-42: UndefinedDocblockClass
src/product/BillingRegistry.php:42:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
[failure] 60-60: UndefinedClass
src/product/BillingRegistry.php:60:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
🔇 Additional comments (3)
src/product/BehaviorInterface.php (1)
7-15: Ensure correct namespace usage
Static analysis indicates that hiqdev\billing\registry\domain\TariffType might be missing or incorrectly referenced. Verify that the namespace path to TariffType is valid or adjust your use statement.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 12-12: UndefinedClass
src/product/BehaviorInterface.php:12:35: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\domain\TariffType does not exist (see https://psalm.dev/019)
[failure] 14-14: UndefinedClass
src/product/BehaviorInterface.php:14:38: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\domain\TariffType does not exist (see https://psalm.dev/019)
src/product/BillingRegistry.php (1)
78-93: Clarify fallback behavior
If getBehavior returns null when no match is found, it may be beneficial to log or throw an exception, depending on usage. This ensures calling code is aware if the behavior truly doesn't exist.
src/product/PriceTypeDefinition.php (1)
20-31: 🛠️ Refactor suggestion
Initialize properties to prevent undefined access
Properties should be initialized with default values to prevent undefined property access.
Apply this diff:
- private UnitInterface $unit;
+ private UnitInterface $unit;
- private string $description;
+ private string $description = '';
- private QuantityFormatterDefinition $quantityFormatterDefinition;
+ private ?QuantityFormatterDefinition $quantityFormatterDefinition = null;
- private InvoiceRepresentationCollection $invoiceCollection;
+ private InvoiceRepresentationCollection $invoiceCollection;
- private PriceTypeDefinitionBehaviourCollection $behaviorCollection;
+ private PriceTypeDefinitionBehaviourCollection $behaviorCollection;
- private Aggregate $aggregate;
+ private ?Aggregate $aggregate = null;Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/product/BehaviorNotFoundException.php (1)
7-9: Add a docblock to clarify usage contextA short docblock explaining when or why this exception is thrown would make the code more self-documenting. For instance, note that it is thrown when a requested behavior class is not registered or cannot be located.
src/product/BillingRegistry.php (2)
78-93: Align naming conventions for “Behavior” vs. “Behaviour”
The exception message uses British spelling, while the class is named in American spelling. For consistency, update the message or rename the class. Also, since you always throw an exception on failure, returning a nullable?BehaviorInterfacemight be misleading.- throw new BehaviorNotFoundException('Behaviour was not found'); + throw new BehaviorNotFoundException('Behavior was not found');
95-112: Add docblock and consider deduplication
Given thatgetBehaviorsiterates over both tariff types and price types, it may yield the same behavior multiple times if it is declared at multiple levels. If that’s undesirable, consider filtering or deduplicating. Also, add a docblock for clarity:/** + * Retrieves a generator of all behaviors matching the specified class, + * searching in both tariff types and price types. + * + * @param string $behaviorClassWrapper + * @return \Generator<BehaviorInterface> + */ public function getBehaviors(string $behaviorClassWrapper): \Generator {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/product/BehaviorNotFoundException.php(1 hunks)src/product/BillingRegistry.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/BillingRegistry.php
[failure] 14-14: MissingDependency
src/product/BillingRegistry.php:14:5: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
[failure] 14-14: MissingDependency
src/product/BillingRegistry.php:14:13: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
[failure] 17-17: MissingDependency
src/product/BillingRegistry.php:17:35: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
[failure] 34-34: MissingDependency
src/product/BillingRegistry.php:34:22: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
[failure] 42-42: UndefinedDocblockClass
src/product/BillingRegistry.php:42:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
[failure] 60-60: UndefinedClass
src/product/BillingRegistry.php:60:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
🔇 Additional comments (6)
src/product/BillingRegistry.php (6)
17-24: Prevent duplicate tariff-type entries
This comment is identical to a past suggestion about adding validation for duplicates. Consider checking for an existing tariff type with the same ID before adding a new one.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 17-17: MissingDependency
src/product/BillingRegistry.php:17:35: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
26-30: Locking mechanism confirmed
The locking mechanism is straightforward and effectively prevents further modifications.
31-38: Error handling for price retrieval
The method inherits any errors arising from each tariffType->withPrices(). A previous review recommended adding error handling and clarifying the return type. Consider reintroducing or adopting that approach if applicable.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 34-34: MissingDependency
src/product/BillingRegistry.php:34:22: MissingDependency: hiqdev\php\billing\product\TariffTypeDefinition depends on class or interface hiqdev\php\billing\product\parentnodedefinitioninterface that does not exist (see https://psalm.dev/157)
40-56: Docblock references an unknown interface?
Static analysis flags RepresentationInterface in the docblock as undefined. Confirm that this interface is available and loaded in the codebase.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 42-42: UndefinedDocblockClass
src/product/BillingRegistry.php:42:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
73-76: Conversion function usage
This private method simply delegates to Type::anyId. That’s acceptable, but if your domain logic requires advanced string-to-type conversion later, consider adding validation or caching for performance.
58-71: 🛠️ Refactor suggestion
Contextualize the exception message
When throwing a QuantityFormatterNotFoundException, include the given $type to aid debugging:
-throw new QuantityFormatterNotFoundException('Quantity formatter not found');
+throw new QuantityFormatterNotFoundException(
+ sprintf("Quantity formatter not found for type '%s'", $type->getId())
+);Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 60-60: UndefinedClass
src/product/BillingRegistry.php:60:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
src/product/BillingRegistry.php
Outdated
| use hiqdev\billing\registry\invoice\RepresentationInterface; | ||
| use hiqdev\billing\registry\quantity\formatter\QuantityFormatterNotFoundException; | ||
| use hiqdev\billing\registry\quantity\FractionQuantityData; | ||
| use hiqdev\php\billing\type\Type; | ||
| use hiqdev\php\billing\type\TypeInterface; |
There was a problem hiding this comment.
💡 Codebase verification
Incorrect namespace paths in imports
The imports are using incorrect namespace paths. Based on the codebase analysis:
DocumentRepresentationInterfaceis actually inhiqdev\php\billing\productnamespace, nothiqdev\billing\registry\invoice- The other classes are imported from
hiqdev\billing\registrynamespace, but this dependency is not listed in composer.json
The imports should be updated to use the correct namespaces. Here are the specific issues:
use hiqdev\billing\registry\invoice\RepresentationInterfaceshould beuse hiqdev\php\billing\product\DocumentRepresentationInterfaceuse hiqdev\billing\registry\quantity\formatter\QuantityFormatterNotFoundExceptionanduse hiqdev\billing\registry\quantity\FractionQuantityDataappear to be from an external package that needs to be added as a dependency
🔗 Analysis chain
Verify import references or rename them if necessary
Static analysis flags these imports as undefined modules. Confirm that the hiqdev\billing\registry\invoice\RepresentationInterface, hiqdev\billing\registry\quantity\formatter\QuantityFormatterNotFoundException, and hiqdev\billing\registry\quantity\FractionQuantityData classes exist in the project or rename them accordingly.
Use the following script to locate all references and verify their existence:
Let me try a different approach to verify these imports by searching for any references to these classes in the codebase.
Let me check the composer.json and the actual implementations of these interfaces/classes to verify their correct namespaces.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Looking for the definitions
ast-grep --pattern $'class RepresentationInterface { $$$ }'
ast-grep --pattern $'class QuantityFormatterNotFoundException { $$$ }'
ast-grep --pattern $'class FractionQuantityData { $$$ }'
Length of output: 190
Script:
#!/bin/bash
# Search for any references to these classes in the codebase
rg -l "RepresentationInterface" --type php
rg -l "QuantityFormatterNotFoundException" --type php
rg -l "FractionQuantityData" --type php
# Check if there are any vendor directories that might contain these dependencies
fd vendor -t d
# Look for composer.json to check dependencies
fd composer.json
Length of output: 366
Script:
#!/bin/bash
# Check composer.json for dependencies
cat composer.json
# Check the actual implementations
cat src/product/DocumentRepresentationInterface.php
cat src/product/PriceTypeDefinition.php
# Look for any namespace declarations in these files
rg -A 1 "namespace" src/product/DocumentRepresentationInterface.php src/product/PriceTypeDefinition.php
Length of output: 7504
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/product/ParentNodeDefinitionInterface.php (1)
5-8: Consider adding a docblock and clarifying the method purpose.
It would be beneficial to document the contract ofwithBehaviors()for improved maintainability and clarity, especially regarding what type ofBehaviorCollectiongets returned and any side effects. IfBehaviorCollectionresides in a different namespace, ensure that it’s properly imported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/product/ParentNodeDefinitionInterface.php(1 hunks)
🔇 Additional comments (1)
src/product/ParentNodeDefinitionInterface.php (1)
1-1: Good use of strict typing.
This ensures type safety across the file, preventing unexpected type coercion issues.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/product/PriceTypeDefinition.php (3)
5-16:⚠️ Potential issueAdd missing billing-registry dependency
Multiple classes from the
hiqdev\billing\registrynamespace are not found. This is a dependency issue.Add the required dependency to your composer.json as mentioned in the previous comment.
74-75:⚠️ Potential issueImplement class existence validation
The TODO comment indicates missing validation for the formatter class.
Implement the class existence check as suggested in the previous review comments.
101-104:⚠️ Potential issueImplement measuredWith method logic
The
measuredWithmethod is currently a no-op, returning$thiswithout any implementation.Either implement the measurement logic or remove the method if it's not needed, as mentioned in previous review comments.
🧹 Nitpick comments (2)
src/product/BehaviorInterface.php (1)
7-15: Add method-level documentationThe interface methods lack PHPDoc documentation explaining their purpose and parameters.
Apply this diff:
interface BehaviorInterface { + /** + * Sets the tariff type for this behavior. + * + * @param TariffType $tariffTypeName The tariff type to set + */ public function setTariffType(TariffType $tariffTypeName): void; + /** + * Gets the current tariff type. + * + * @return TariffType The current tariff type + */ public function getTariffType(): TariffType; }🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 12-12: UndefinedClass
src/product/BehaviorInterface.php:12:35: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\Domain\Model\TariffType does not exist (see https://psalm.dev/019)
[failure] 14-14: UndefinedClass
src/product/BehaviorInterface.php:14:38: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\Domain\Model\TariffType does not exist (see https://psalm.dev/019)src/product/BehaviorCollection.php (1)
7-7: Add generic type hints for IteratorAggregateThe class implements IteratorAggregate but lacks the generic type parameter.
Apply this diff:
-class BehaviorCollection implements \IteratorAggregate +class BehaviorCollection implements \IteratorAggregate<int, BehaviorInterface>🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 7-7: MissingTemplateParam
src/product/BehaviorCollection.php:7:37: MissingTemplateParam: hiqdev\php\billing\product\BehaviorCollection has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/product/BehaviorCollection.php(1 hunks)src/product/BehaviorInterface.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)src/product/PriceTypeDefinitionCollection.php(1 hunks)src/product/TariffTypeDefinition.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/product/TariffTypeDefinition.php
- src/product/PriceTypeDefinitionCollection.php
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/PriceTypeDefinition.php
[failure] 24-24: UndefinedClass
src/product/PriceTypeDefinition.php:24:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\formatter\QuantityFormatterDefinition does not exist (see https://psalm.dev/019)
[failure] 26-26: UndefinedClass
src/product/PriceTypeDefinition.php:26:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 28-28: UndefinedClass
src/product/PriceTypeDefinition.php:28:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\PriceTypeDefinitionBehaviourCollection does not exist (see https://psalm.dev/019)
src/product/BehaviorCollection.php
[failure] 7-7: MissingTemplateParam
src/product/BehaviorCollection.php:7:37: MissingTemplateParam: hiqdev\php\billing\product\BehaviorCollection has missing template params when extending IteratorAggregate, expecting 2 (see https://psalm.dev/182)
[failure] 12-12: UndefinedClass
src/product/BehaviorCollection.php:12:33: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\Domain\Model\TariffType does not exist (see https://psalm.dev/019)
src/product/BehaviorInterface.php
[failure] 12-12: UndefinedClass
src/product/BehaviorInterface.php:12:35: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\Domain\Model\TariffType does not exist (see https://psalm.dev/019)
[failure] 14-14: UndefinedClass
src/product/BehaviorInterface.php:14:38: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\Domain\Model\TariffType does not exist (see https://psalm.dev/019)
🔇 Additional comments (1)
src/product/BehaviorCollection.php (1)
24-31: Method implementation looks good
The attach method correctly sets the tariff type and maintains method chaining.
src/product/BehaviorInterface.php
Outdated
|
|
||
| namespace hiqdev\php\billing\product; | ||
|
|
||
| use hiqdev\billing\registry\Domain\Model\TariffType; |
There was a problem hiding this comment.
Add missing dependency for TariffType class
The TariffType class from hiqdev\billing\registry\Domain\Model namespace is not found. This appears to be a missing dependency issue.
Add the required dependency to your composer.json:
{
"require": {
"hiqdev/billing-registry": "^1.0"
}
}
src/product/PriceTypeDefinition.php
Outdated
| * це параметер визначає агрегатну функцію яка застосовується для щоденно записаних ресурсів щоб визнизначти | ||
| * місячне споживання за яке потрібно пробілити клієнта | ||
| * |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Translate documentation to English
The PHPDoc comment is written in a non-English language. For better maintainability and collaboration, documentation should be in English.
Apply this diff:
- * це параметер визначає агрегатну функцію яка застосовується для щоденно записаних ресурсів щоб визнизначти
- * місячне споживання за яке потрібно пробілити клієнта
+ * This parameter defines the aggregate function that is applied to daily recorded resources
+ * to determine the monthly consumption for which the client needs to be billed📝 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.
| * це параметер визначає агрегатну функцію яка застосовується для щоденно записаних ресурсів щоб визнизначти | |
| * місячне споживання за яке потрібно пробілити клієнта | |
| * | |
| * This parameter defines the aggregate function that is applied to daily recorded resources | |
| * to determine the monthly consumption for which the client needs to be billed | |
| * |
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 (2)
src/product/BillingRegistry.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/BillingRegistry.php
[failure] 42-42: UndefinedDocblockClass
src/product/BillingRegistry.php:42:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
[failure] 60-60: UndefinedClass
src/product/BillingRegistry.php:60:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
[failure] 98-98: UndefinedClass
src/product/BillingRegistry.php:98:22: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\TariffTypeBehaviorCollection does not exist (see https://psalm.dev/019)
src/product/PriceTypeDefinition.php
[failure] 24-24: UndefinedClass
src/product/PriceTypeDefinition.php:24:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\formatter\QuantityFormatterDefinition does not exist (see https://psalm.dev/019)
[failure] 26-26: UndefinedClass
src/product/PriceTypeDefinition.php:26:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
[failure] 28-28: UndefinedClass
src/product/PriceTypeDefinition.php:28:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\PriceTypeDefinitionBehaviourCollection does not exist (see https://psalm.dev/019)
🔇 Additional comments (11)
src/product/BillingRegistry.php (8)
11-15: LGTM! Properties are well-defined.
The class declaration and properties are properly typed and documented.
26-29: LGTM! Simple and effective implementation.
The lock method correctly prevents further modifications to the registry.
31-38: 🛠️ Refactor suggestion
Add error handling and improve return type specificity.
The method should handle potential errors from withPrices() and provide a more specific return type hint.
-public function priceTypes(): \Generator
+/**
+ * @return \Generator<PriceTypeDefinition>
+ * @throws \RuntimeException When price retrieval fails
+ */
+public function priceTypes(): \Generator
{
foreach ($this->tariffTypes as $tariffType) {
- foreach ($tariffType->withPrices() as $priceTypeDefinition) {
- yield $priceTypeDefinition;
+ try {
+ foreach ($tariffType->withPrices() as $priceTypeDefinition) {
+ yield $priceTypeDefinition;
+ }
+ } catch (\Throwable $e) {
+ throw new \RuntimeException(
+ sprintf("Failed to retrieve prices for tariff type '%s'", $tariffType->getId()),
+ 0,
+ $e
+ );
}
}
}Likely invalid or redundant comment.
17-24: 🛠️ Refactor suggestion
Add validation for duplicate tariff types.
While the lock check is good, the method should also prevent duplicate tariff types to maintain data integrity.
public function addTariffType(TariffTypeDefinition $tariffType): void
{
if ($this->locked) {
throw new \RuntimeException("BillingRegistry is locked and cannot be modified.");
}
+ foreach ($this->tariffTypes as $existingTariff) {
+ if ($existingTariff->getId() === $tariffType->getId()) {
+ throw new \RuntimeException(
+ sprintf("Tariff type with ID '%s' already exists.", $tariffType->getId())
+ );
+ }
+ }
+
$this->tariffTypes[] = $tariffType;
}Likely invalid or redundant comment.
58-71: 🛠️ Refactor suggestion
Improve error handling and input validation.
The method needs better error messages and input validation.
public function createQuantityFormatter(
string $type,
FractionQuantityData $data,
): array {
+ if (empty($type)) {
+ throw new \InvalidArgumentException('Type cannot be empty');
+ }
+
$type = $this->convertStringTypeToType($type);
foreach ($this->priceTypes() as $priceTypeDefinition) {
if ($priceTypeDefinition->hasType($type)) {
return $priceTypeDefinition->createQuantityFormatter($data);
}
}
- throw new QuantityFormatterNotFoundException('Quantity formatter not found');
+ throw new QuantityFormatterNotFoundException(
+ sprintf("Quantity formatter not found for type '%s'", $type)
+ );
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 60-60: UndefinedClass
src/product/BillingRegistry.php:60:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
40-56: 🛠️ Refactor suggestion
Add validation for representation class existence.
The method should validate that the provided class exists and implements RepresentationInterface.
public function getRepresentationsByType(string $representationClass): array
{
+ if (!class_exists($representationClass)) {
+ throw new \InvalidArgumentException(
+ sprintf("Class '%s' does not exist", $representationClass)
+ );
+ }
+ if (!is_subclass_of($representationClass, RepresentationInterface::class)) {
+ throw new \InvalidArgumentException(
+ sprintf("Class '%s' must implement RepresentationInterface", $representationClass)
+ );
+ }
+
$representations = [];
foreach ($this->priceTypes() as $priceTypeDefinition) {
foreach ($priceTypeDefinition->documentRepresentation() as $representation) {Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 42-42: UndefinedDocblockClass
src/product/BillingRegistry.php:42:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
5-8:
Add missing dependency for registry classes.
The code imports several classes from the hiqdev\billing\registry namespace, but this dependency appears to be missing from the project. This will cause compilation failures.
Add the required dependency to your composer.json:
{
"require": {
+ "hiqdev/billing-registry": "^1.0"
}
}Likely invalid or redundant comment.
78-112: 🛠️ Refactor suggestion
Add behavior class validation and improve error messages.
The methods should validate the behavior class wrapper and provide more informative error messages.
public function getBehavior(string $type, string $behaviorClassWrapper): BehaviorInterface
{
+ if (!class_exists($behaviorClassWrapper)) {
+ throw new \InvalidArgumentException(
+ sprintf("Behavior class '%s' does not exist", $behaviorClassWrapper)
+ );
+ }
+ if (!is_subclass_of($behaviorClassWrapper, BehaviorInterface::class)) {
+ throw new \InvalidArgumentException(
+ sprintf("Class '%s' must implement BehaviorInterface", $behaviorClassWrapper)
+ );
+ }
+
$type = $this->convertStringTypeToType($type);
foreach ($this->priceTypes() as $priceTypeDefinition) {
if ($priceTypeDefinition->hasType($type)) {
foreach ($priceTypeDefinition->withBehaviors() as $behavior) {
if ($behavior instanceof $behaviorClassWrapper) {
return $behavior;
}
}
}
}
- throw new BehaviorNotFoundException('Behaviour was not found');
+ throw new BehaviorNotFoundException(
+ sprintf("Behavior of class '%s' not found for type '%s'", $behaviorClassWrapper, $type)
+ );
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 98-98: UndefinedClass
src/product/BillingRegistry.php:98:22: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\TariffTypeBehaviorCollection does not exist (see https://psalm.dev/019)
src/product/PriceTypeDefinition.php (3)
127-129: 🛠️ Refactor suggestion
Translate documentation to English.
Documentation should be in English for better maintainability and collaboration.
- /**
- * це параметер визначає агрегатну функцію яка застосовується для щоденно записаних ресурсів щоб визнизначти
- * місячне споживання за яке потрібно пробілити клієнта
+ /**
+ * This parameter defines the aggregate function that is applied to daily recorded resources
+ * to determine the monthly consumption for which the client needs to be billedLikely invalid or redundant comment.
73-78: 🛠️ Refactor suggestion
Implement TODO: Add class existence validation.
The TODO comment indicates missing validation for the formatter class existence.
public function quantityFormatter(string $formatterClass, $fractionUnit = null): self
{
- // TODO: check if formatterClass exists
+ if (!class_exists($formatterClass)) {
+ throw new \InvalidArgumentException(
+ sprintf('Formatter class "%s" does not exist', $formatterClass)
+ );
+ }
+ if (!is_subclass_of($formatterClass, QuantityFormatterInterface::class)) {
+ throw new \InvalidArgumentException(
+ sprintf('Class "%s" must implement QuantityFormatterInterface', $formatterClass)
+ );
+ }
$this->quantityFormatterDefinition = new QuantityFormatterDefinition($formatterClass, $fractionUnit);
return $this;
}Likely invalid or redundant comment.
90-94: 🛠️ Refactor suggestion
Add validation in end() method.
The end() method should validate that all required properties are set before returning.
public function end(): PriceTypeDefinitionCollection
{
- // Validate the PriceType and lock its state
+ $requiredProps = ['unit', 'description', 'quantityFormatterDefinition'];
+ foreach ($requiredProps as $prop) {
+ if (!isset($this->$prop)) {
+ throw new \RuntimeException(sprintf("Required property '%s' is not set", $prop));
+ }
+ }
return $this->parent;
}Likely invalid or redundant comment.
src/product/PriceTypeDefinition.php
Outdated
| public function measuredWith(\hiqdev\billing\registry\measure\RcpTrafCollector $param): self | ||
| { | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Remove unused measuredWith method
The measuredWith method in PriceTypeDefinition.php is not used anywhere in the codebase, and its implementation is a no-op that just returns $this. Since both the method and its parameter type RcpTrafCollector are only referenced in this single location, it's safe to remove this unused method.
🔗 Analysis chain
Implement measuredWith method or remove it.
The measuredWith method is currently a no-op. Either implement the measurement logic or remove the method if it's not needed.
Let me check if this method is used elsewhere:
Let me try to find any usage of this method or the collector class it depends on.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for uses of measuredWith method
rg "->measuredWith\(" --type php
Length of output: 439
Script:
#!/bin/bash
# Search for uses of measuredWith method with proper escaping
rg "measuredWith" --type php
# Also check for any references to RcpTrafCollector class
rg "RcpTrafCollector" --type php
Length of output: 327
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/product/BillingRegistry.php (2)
13-24: Enhance type safety and data validationThe tariff types storage implementation could be improved:
- The property docblock could be more specific
- Consider validating for duplicate tariff types before adding
Apply these improvements:
- /** @var TariffTypeDefinition[] */ + /** @var array<int|string, TariffTypeDefinition> */ private array $tariffTypes = []; public function addTariffType(TariffTypeDefinition $tariffType): void { if ($this->locked) { throw new \RuntimeException("BillingRegistry is locked and cannot be modified."); } + + foreach ($this->tariffTypes as $existingTariff) { + if ($existingTariff->getId() === $tariffType->getId()) { + throw new \RuntimeException( + sprintf('Tariff type with ID "%s" already exists', $tariffType->getId()) + ); + } + } $this->tariffTypes[] = $tariffType; }
73-76: Add input validation and documentationThe type conversion helper needs input validation and better documentation.
Apply these improvements:
+ /** + * Converts a string type identifier to a TypeInterface instance. + * + * @param string $type The type identifier + * @return TypeInterface + * @throws \InvalidArgumentException When type is empty + */ private function convertStringTypeToType(string $type): TypeInterface { + if (empty($type)) { + throw new \InvalidArgumentException('Type cannot be empty'); + } + return Type::anyId($type); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/product/BillingRegistry.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/BillingRegistry.php
[failure] 42-42: UndefinedDocblockClass
src/product/BillingRegistry.php:42:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
[failure] 60-60: UndefinedClass
src/product/BillingRegistry.php:60:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
[failure] 104-104: UndefinedClass
src/product/BillingRegistry.php:104:22: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\TariffTypeBehaviorCollection does not exist (see https://psalm.dev/019)
🔇 Additional comments (3)
src/product/BillingRegistry.php (3)
5-8:
Fix incorrect import paths
The imports from hiqdev\billing\registry namespace are incorrect. These classes should be imported from the hiqdev\php\billing namespace according to the project structure.
Apply this diff to fix the imports:
-use hiqdev\billing\registry\invoice\RepresentationInterface;
-use hiqdev\billing\registry\quantity\formatter\QuantityFormatterNotFoundException;
-use hiqdev\billing\registry\quantity\FractionQuantityData;
+use hiqdev\php\billing\product\DocumentRepresentationInterface;
+use hiqdev\php\billing\product\formatter\QuantityFormatterNotFoundException;
+use hiqdev\php\billing\product\quantity\FractionQuantityData;Likely invalid or redundant comment.
31-38: 🛠️ Refactor suggestion
Add error handling and improve return type documentation
The price types generator needs better error handling and documentation.
Apply these improvements:
+ /**
+ * @return \Generator<int, PriceTypeDefinition>
+ * @throws \RuntimeException When price retrieval fails
+ */
public function priceTypes(): \Generator
{
foreach ($this->tariffTypes as $tariffType) {
+ try {
foreach ($tariffType->withPrices() as $priceTypeDefinition) {
yield $priceTypeDefinition;
}
+ } catch (\Throwable $e) {
+ throw new \RuntimeException(
+ sprintf('Failed to retrieve prices for tariff type: %s', $tariffType->getId()),
+ 0,
+ $e
+ );
+ }
}
}Likely invalid or redundant comment.
58-71: 🛠️ Refactor suggestion
Improve error handling and documentation
The quantity formatter creation needs better input validation, error messages, and documentation.
Apply these improvements:
+ /**
+ * @param string $type The type identifier
+ * @param FractionQuantityData $data The quantity data
+ * @return array{formatter: callable, params: array} The formatter configuration
+ * @throws QuantityFormatterNotFoundException When no formatter is found
+ * @throws \InvalidArgumentException When type is empty
+ */
public function createQuantityFormatter(
string $type,
FractionQuantityData $data,
): array {
+ if (empty($type)) {
+ throw new \InvalidArgumentException('Type cannot be empty');
+ }
+
$type = $this->convertStringTypeToType($type);
foreach ($this->priceTypes() as $priceTypeDefinition) {
if ($priceTypeDefinition->hasType($type)) {
return $priceTypeDefinition->createQuantityFormatter($data);
}
}
- throw new QuantityFormatterNotFoundException('Quantity formatter not found');
+ throw new QuantityFormatterNotFoundException(
+ sprintf('Quantity formatter not found for type "%s"', $type)
+ );
}Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 60-60: UndefinedClass
src/product/BillingRegistry.php:60:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
src/product/BillingRegistry.php
Outdated
| /** | ||
| * @param string $representationClass | ||
| * @return RepresentationInterface[] | ||
| */ | ||
| public function getRepresentationsByType(string $representationClass): array | ||
| { | ||
| $representations = []; | ||
| foreach ($this->priceTypes() as $priceTypeDefinition) { | ||
| foreach ($priceTypeDefinition->documentRepresentation() as $representation) { | ||
| if ($representation instanceof $representationClass) { | ||
| $representations[] = $representation; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return $representations; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and fix return type hint
The method needs validation for the representation class and updated return type documentation.
Apply these improvements:
/**
* @param string $representationClass
- * @return RepresentationInterface[]
+ * @return DocumentRepresentationInterface[]
+ * @throws \InvalidArgumentException When representation class is invalid
*/
public function getRepresentationsByType(string $representationClass): array
{
+ if (!class_exists($representationClass)) {
+ throw new \InvalidArgumentException(
+ sprintf('Class "%s" does not exist', $representationClass)
+ );
+ }
+ if (!is_subclass_of($representationClass, DocumentRepresentationInterface::class)) {
+ throw new \InvalidArgumentException(
+ sprintf('Class "%s" must implement DocumentRepresentationInterface', $representationClass)
+ );
+ }
+
$representations = [];📝 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.
| /** | |
| * @param string $representationClass | |
| * @return RepresentationInterface[] | |
| */ | |
| public function getRepresentationsByType(string $representationClass): array | |
| { | |
| $representations = []; | |
| foreach ($this->priceTypes() as $priceTypeDefinition) { | |
| foreach ($priceTypeDefinition->documentRepresentation() as $representation) { | |
| if ($representation instanceof $representationClass) { | |
| $representations[] = $representation; | |
| } | |
| } | |
| } | |
| return $representations; | |
| } | |
| /** | |
| * @param string $representationClass | |
| * @return DocumentRepresentationInterface[] | |
| * @throws \InvalidArgumentException When representation class is invalid | |
| */ | |
| public function getRepresentationsByType(string $representationClass): array | |
| { | |
| if (!class_exists($representationClass)) { | |
| throw new \InvalidArgumentException( | |
| sprintf('Class "%s" does not exist', $representationClass) | |
| ); | |
| } | |
| if (!is_subclass_of($representationClass, DocumentRepresentationInterface::class)) { | |
| throw new \InvalidArgumentException( | |
| sprintf('Class "%s" must implement DocumentRepresentationInterface', $representationClass) | |
| ); | |
| } | |
| $representations = []; | |
| foreach ($this->priceTypes() as $priceTypeDefinition) { | |
| foreach ($priceTypeDefinition->documentRepresentation() as $representation) { | |
| if ($representation instanceof $representationClass) { | |
| $representations[] = $representation; | |
| } | |
| } | |
| } | |
| return $representations; | |
| } |
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 42-42: UndefinedDocblockClass
src/product/BillingRegistry.php:42:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
src/product/BillingRegistry.php
Outdated
| /** | ||
| * @param string $type - full type like 'overuse,lb_capacity_unit' | ||
| * @param string $behaviorClassWrapper | ||
| * @return BehaviorInterface | ||
| * @throws BehaviorNotFoundException | ||
| */ | ||
| public function getBehavior(string $type, string $behaviorClassWrapper): BehaviorInterface | ||
| { | ||
| $type = $this->convertStringTypeToType($type); | ||
|
|
||
| foreach ($this->priceTypes() as $priceTypeDefinition) { | ||
| if ($priceTypeDefinition->hasType($type)) { | ||
| foreach ($priceTypeDefinition->withBehaviors() as $behavior) { | ||
| if ($behavior instanceof $behaviorClassWrapper) { | ||
| return $behavior; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| throw new BehaviorNotFoundException('Behaviour was not found'); | ||
| } | ||
|
|
||
| public function getBehaviors(string $behaviorClassWrapper): \Generator | ||
| { | ||
| foreach ($this->tariffTypes as $tariffType) { | ||
| foreach ($tariffType->withBehaviors() as $behavior) { | ||
| if ($behavior instanceof $behaviorClassWrapper) { | ||
| yield $behavior; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| foreach ($this->priceTypes() as $priceTypeDefinition) { | ||
| foreach ($priceTypeDefinition->withBehaviors() as $behavior) { | ||
| if ($behavior instanceof $behaviorClassWrapper) { | ||
| yield $behavior; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor behavior management for better maintainability
The behavior management implementation needs several improvements:
- Input validation
- Better error messages
- Code deduplication
- Better documentation
Apply these improvements:
+ /**
+ * @param string $behaviorClassWrapper The behavior class to match
+ * @return \Generator<int, BehaviorInterface>
+ * @throws \InvalidArgumentException When behavior class is invalid
+ */
+ private function findBehaviorsInCollection(iterable $collection, string $behaviorClassWrapper): \Generator
+ {
+ foreach ($collection as $behavior) {
+ if ($behavior instanceof $behaviorClassWrapper) {
+ yield $behavior;
+ }
+ }
+ }
/**
* @param string $type - full type like 'overuse,lb_capacity_unit'
* @param string $behaviorClassWrapper
* @return BehaviorInterface
* @throws BehaviorNotFoundException
+ * @throws \InvalidArgumentException When parameters are invalid
*/
public function getBehavior(string $type, string $behaviorClassWrapper): BehaviorInterface
{
+ if (empty($type)) {
+ throw new \InvalidArgumentException('Type cannot be empty');
+ }
+ if (!class_exists($behaviorClassWrapper)) {
+ throw new \InvalidArgumentException(
+ sprintf('Behavior class "%s" does not exist', $behaviorClassWrapper)
+ );
+ }
+
$type = $this->convertStringTypeToType($type);
foreach ($this->priceTypes() as $priceTypeDefinition) {
if ($priceTypeDefinition->hasType($type)) {
- foreach ($priceTypeDefinition->withBehaviors() as $behavior) {
- if ($behavior instanceof $behaviorClassWrapper) {
- return $behavior;
- }
+ foreach ($this->findBehaviorsInCollection($priceTypeDefinition->withBehaviors(), $behaviorClassWrapper) as $behavior) {
+ return $behavior;
}
}
}
- throw new BehaviorNotFoundException('Behaviour was not found');
+ throw new BehaviorNotFoundException(
+ sprintf('Behavior of class "%s" not found for type "%s"', $behaviorClassWrapper, $type)
+ );
}
+ /**
+ * @param string $behaviorClassWrapper The behavior class to match
+ * @return \Generator<int, BehaviorInterface>
+ * @throws \InvalidArgumentException When behavior class is invalid
+ */
public function getBehaviors(string $behaviorClassWrapper): \Generator
{
+ if (!class_exists($behaviorClassWrapper)) {
+ throw new \InvalidArgumentException(
+ sprintf('Behavior class "%s" does not exist', $behaviorClassWrapper)
+ );
+ }
+
foreach ($this->tariffTypes as $tariffType) {
- foreach ($tariffType->withBehaviors() as $behavior) {
- if ($behavior instanceof $behaviorClassWrapper) {
- yield $behavior;
- }
- }
+ yield from $this->findBehaviorsInCollection($tariffType->withBehaviors(), $behaviorClassWrapper);
}
foreach ($this->priceTypes() as $priceTypeDefinition) {
- foreach ($priceTypeDefinition->withBehaviors() as $behavior) {
- if ($behavior instanceof $behaviorClassWrapper) {
- yield $behavior;
- }
- }
+ yield from $this->findBehaviorsInCollection($priceTypeDefinition->withBehaviors(), $behaviorClassWrapper);
}
}📝 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.
| /** | |
| * @param string $type - full type like 'overuse,lb_capacity_unit' | |
| * @param string $behaviorClassWrapper | |
| * @return BehaviorInterface | |
| * @throws BehaviorNotFoundException | |
| */ | |
| public function getBehavior(string $type, string $behaviorClassWrapper): BehaviorInterface | |
| { | |
| $type = $this->convertStringTypeToType($type); | |
| foreach ($this->priceTypes() as $priceTypeDefinition) { | |
| if ($priceTypeDefinition->hasType($type)) { | |
| foreach ($priceTypeDefinition->withBehaviors() as $behavior) { | |
| if ($behavior instanceof $behaviorClassWrapper) { | |
| return $behavior; | |
| } | |
| } | |
| } | |
| } | |
| throw new BehaviorNotFoundException('Behaviour was not found'); | |
| } | |
| public function getBehaviors(string $behaviorClassWrapper): \Generator | |
| { | |
| foreach ($this->tariffTypes as $tariffType) { | |
| foreach ($tariffType->withBehaviors() as $behavior) { | |
| if ($behavior instanceof $behaviorClassWrapper) { | |
| yield $behavior; | |
| } | |
| } | |
| } | |
| foreach ($this->priceTypes() as $priceTypeDefinition) { | |
| foreach ($priceTypeDefinition->withBehaviors() as $behavior) { | |
| if ($behavior instanceof $behaviorClassWrapper) { | |
| yield $behavior; | |
| } | |
| } | |
| } | |
| } | |
| /** | |
| * @param string $behaviorClassWrapper The behavior class to match | |
| * @return \Generator<int, BehaviorInterface> | |
| * @throws \InvalidArgumentException When behavior class is invalid | |
| */ | |
| private function findBehaviorsInCollection(iterable $collection, string $behaviorClassWrapper): \Generator | |
| { | |
| foreach ($collection as $behavior) { | |
| if ($behavior instanceof $behaviorClassWrapper) { | |
| yield $behavior; | |
| } | |
| } | |
| } | |
| /** | |
| * @param string $type - full type like 'overuse,lb_capacity_unit' | |
| * @param string $behaviorClassWrapper | |
| * @return BehaviorInterface | |
| * @throws BehaviorNotFoundException | |
| * @throws \InvalidArgumentException When parameters are invalid | |
| */ | |
| public function getBehavior(string $type, string $behaviorClassWrapper): BehaviorInterface | |
| { | |
| if (empty($type)) { | |
| throw new \InvalidArgumentException('Type cannot be empty'); | |
| } | |
| if (!class_exists($behaviorClassWrapper)) { | |
| throw new \InvalidArgumentException( | |
| sprintf('Behavior class "%s" does not exist', $behaviorClassWrapper) | |
| ); | |
| } | |
| $type = $this->convertStringTypeToType($type); | |
| foreach ($this->priceTypes() as $priceTypeDefinition) { | |
| if ($priceTypeDefinition->hasType($type)) { | |
| foreach ($this->findBehaviorsInCollection($priceTypeDefinition->withBehaviors(), $behaviorClassWrapper) as $behavior) { | |
| return $behavior; | |
| } | |
| } | |
| } | |
| throw new BehaviorNotFoundException( | |
| sprintf('Behavior of class "%s" not found for type "%s"', $behaviorClassWrapper, $type) | |
| ); | |
| } | |
| /** | |
| * @param string $behaviorClassWrapper The behavior class to match | |
| * @return \Generator<int, BehaviorInterface> | |
| * @throws \InvalidArgumentException When behavior class is invalid | |
| */ | |
| public function getBehaviors(string $behaviorClassWrapper): \Generator | |
| { | |
| if (!class_exists($behaviorClassWrapper)) { | |
| throw new \InvalidArgumentException( | |
| sprintf('Behavior class "%s" does not exist', $behaviorClassWrapper) | |
| ); | |
| } | |
| foreach ($this->tariffTypes as $tariffType) { | |
| yield from $this->findBehaviorsInCollection($tariffType->withBehaviors(), $behaviorClassWrapper); | |
| } | |
| foreach ($this->priceTypes() as $priceTypeDefinition) { | |
| yield from $this->findBehaviorsInCollection($priceTypeDefinition->withBehaviors(), $behaviorClassWrapper); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 104-104: UndefinedClass
src/product/BillingRegistry.php:104:22: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\TariffTypeBehaviorCollection does not exist (see https://psalm.dev/019)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/product/AggregateNotFoundException.php (1)
7-9: Consider adding a default error message.While extending
RuntimeExceptionis valid, including a default message or a context parameter could aid debugging. For instance:-class AggregateNotFoundException extends RuntimeException +class AggregateNotFoundException extends RuntimeException +{ + public function __construct(string $type = '') + { + $message = $type !== '' + ? "Aggregate not found for type '$type'" + : "Aggregate not found"; + parent::__construct($message); + } +}src/product/ParentNodeDefinitionInterface.php (1)
5-10: Ensure comprehensive documentation for interface methods.Documenting intent and usage in
withBehaviors()andhasBehavior()allows future maintainers to implement or mock them correctly. Consider adding PHPDoc tags describing parameters and return types in more detail.src/product/BillingRegistry.php (1)
41-44: Adjust docblock for more specific or correct interface references.Static analysis indicates that
@return RepresentationInterface[]may not match an actual, existing interface name. Update docblock references to reflect the correct interface or class name used in production code.🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 43-43: UndefinedDocblockClass
src/product/BillingRegistry.php:43:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)src/product/PriceTypeDefinition.php (3)
48-50: Consider adding input validation for$unit.
$this->unit = Unit::create($unit);will fail if$unitis invalid or empty. Add checks to ensure$unitis valid, and throw an exception if not.
72-75: Resolve TODO on formatter class checks.Line 74 references a TODO to check the formatter’s validity. Add class existence checks and interface checks to ensure the formatter class implements
QuantityFormatterInterface.
137-140: Translate non-English comments to English.Lines 138-140 are written in another language. For better maintainability, convert them to English so that all developers can understand the code’s purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/product/AggregateNotFoundException.php(1 hunks)src/product/BillingRegistry.php(1 hunks)src/product/ParentNodeDefinitionInterface.php(1 hunks)src/product/PriceTypeDefinition.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/BillingRegistry.php
[failure] 43-43: UndefinedDocblockClass
src/product/BillingRegistry.php:43:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
[failure] 61-61: UndefinedClass
src/product/BillingRegistry.php:61:9: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\FractionQuantityData does not exist (see https://psalm.dev/019)
[failure] 105-105: UndefinedClass
src/product/BillingRegistry.php:105:22: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\TariffTypeBehaviorCollection does not exist (see https://psalm.dev/019)
[failure] 121-121: UndefinedClass
src/product/BillingRegistry.php:121:49: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Aggregate does not exist (see https://psalm.dev/019)
src/product/PriceTypeDefinition.php
[failure] 24-24: UndefinedClass
src/product/PriceTypeDefinition.php:24:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\formatter\QuantityFormatterDefinition does not exist (see https://psalm.dev/019)
[failure] 26-26: UndefinedClass
src/product/PriceTypeDefinition.php:26:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\invoice\InvoiceRepresentationCollection does not exist (see https://psalm.dev/019)
🔇 Additional comments (2)
src/product/BillingRegistry.php (2)
7-8: Confirm presence of classes from external package.
QuantityFormatterNotFoundExceptionandFractionQuantityDatapoint tohiqdev\billing\registry\quantity. If these classes live in an external package, ensure composer dependencies are in place. Otherwise, update the namespaces or adapt the code to existing classes.
85-100: 🛠️ Refactor suggestionValidate behavior class.
getBehaviordoesn't validate$behaviorClassWrapperbefore usinginstanceof. Consider adding existence checks and ensuring it implements the expected interface. Avoid runtime failures from invalid classes.public function getBehavior(string $type, string $behaviorClassWrapper): BehaviorInterface { + if (!class_exists($behaviorClassWrapper)) { + throw new \InvalidArgumentException( + "Behavior class '$behaviorClassWrapper' does not exist" + ); + } + $type = $this->convertStringTypeToType($type); ... }Likely invalid or redundant comment.
src/product/BillingRegistry.php
Outdated
| public function getAggregate(string $type): Aggregate | ||
| { | ||
| $type = $this->convertStringTypeToType($type); | ||
|
|
||
| foreach ($this->priceTypes() as $priceTypeDefinition) { | ||
| if ($priceTypeDefinition->hasType($type)) { | ||
| return $priceTypeDefinition->getAggregate(); | ||
| } | ||
| } | ||
|
|
||
| throw new AggregateNotFoundException('Aggregate was not found'); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
The Aggregate class appears to be missing from the codebase
The class Aggregate is imported from hiqdev\billing\registry\product\Aggregate namespace in both BillingRegistry.php and PriceTypeDefinition.php, but:
- The class definition is not found in the codebase
- It's not available in the vendor directory (which is not present in the sandbox)
- The namespace suggests it should be in a package named
hiqdev/billing-registry, but this package is not listed in composer dependencies
This could lead to runtime errors. You should either:
- Add the missing package that contains this class to composer dependencies
- Create the
Aggregateclass in the correct namespace if it's meant to be part of this package - Update the code to use a different class if the dependency was renamed or moved
🔗 Analysis chain
Validate existence of Aggregate class prior to usage.
Aggregate is imported from hiqdev\billing\registry\product\Aggregate, but static analysis flags it as undefined. Ensure the class is correctly included in composer or located in the repository. Otherwise, reference the correct namespace or rename as needed.
Let me search for more information about the Aggregate class.
Let me check if this is a dependency from composer by looking at composer.json.
Let me try one more search with proper escaping to find any references to this class.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the Aggregate class existence
rg "class Aggregate" -t php
Length of output: 130
Script:
#!/bin/bash
# Check for any file named Aggregate.php
fd Aggregate.php
# Check for any usage or import of Aggregate class
rg "use.*Aggregate" -t php
Length of output: 209
Script:
#!/bin/bash
# Check composer.json for the package
cat composer.json
# Also check if there are any other references to the Aggregate class
rg "hiqdev\\billing\\registry\\product\\Aggregate" -t php
Length of output: 3132
Script:
#!/bin/bash
# Search for the Aggregate class definition in vendor directory
fd -t f "Aggregate.php" vendor
# Search for references with proper escaping
rg -l "hiqdev\\\\billing\\\\registry\\\\product\\\\Aggregate"
Length of output: 248
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 121-121: UndefinedClass
src/product/BillingRegistry.php:121:49: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Aggregate does not exist (see https://psalm.dev/019)
src/product/PriceTypeDefinition.php
Outdated
| use hiqdev\billing\registry\behavior\PriceTypeDefinitionBehaviourCollection; | ||
| use hiqdev\billing\registry\Domain\Model\TariffType; | ||
| use hiqdev\billing\registry\invoice\InvoiceRepresentationCollection; | ||
| use hiqdev\billing\registry\product\Aggregate; | ||
| use hiqdev\billing\registry\quantity\formatter\QuantityFormatterDefinition; | ||
| use hiqdev\billing\registry\quantity\formatter\QuantityFormatterFactory; | ||
| use hiqdev\billing\registry\quantity\FractionQuantityData; | ||
| use hiqdev\billing\registry\Domain\Model\Unit\FractionUnit; | ||
| use hiqdev\php\billing\quantity\QuantityFormatterInterface; | ||
| use hiqdev\php\billing\type\TypeInterface; | ||
| use hiqdev\php\units\Unit; | ||
| use hiqdev\php\units\UnitInterface; |
There was a problem hiding this comment.
💡 Codebase verification
Missing required dependency hiqdev/billing-registry
The codebase heavily uses classes from the hiqdev\billing\registry namespace, but this dependency is not listed in the composer.json requirements. This could lead to runtime errors.
- Add
"hiqdev/billing-registry": "^version"to therequiresection incomposer.json - Multiple files depend on this package:
src/product/PriceTypeDefinition.phpsrc/product/TariffTypeDefinition.phpsrc/product/BillingRegistry.php- And others
🔗 Analysis chain
Confirm external dependency availability.
PriceTypeDefinitionBehaviourCollection, InvoiceRepresentationCollection, and similar references to hiqdev\billing\registry\... classes imply an external or separate package. Verify that the correct dependency is installed and configured in composer.json.
Let me try a different approach to verify the dependency.
Let me check if the registry package exists in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check composer.json for references to hiqdev\billing\registry
rg "hiqdev\\\\billing\\\\registry" composer.json
Length of output: 46
Script:
#!/bin/bash
# Check composer.json content to see all dependencies
cat composer.json
Length of output: 2946
Script:
#!/bin/bash
# Check if the registry package exists in the codebase
fd "registry" --type d
# Also check if there are any references to the registry namespace in the codebase
rg "hiqdev\\\\billing\\\\registry" -g "*.php"
Length of output: 2579
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/product/PriceTypeDefinition.php (4)
5-14:⚠️ Potential issueAdd missing dependency
hiqdev/billing-registryMultiple classes are imported from the
hiqdev\billing\registrynamespace, but this dependency appears to be missing fromcomposer.json. This will cause runtime errors.Add the following to your
composer.json:{ "require": { + "hiqdev/billing-registry": "^version" } }
19-29: 🛠️ Refactor suggestionInitialize class properties
Properties should be initialized to prevent undefined property access.
Apply this diff:
- private Unit $unit; + private Unit $unit; - private string $description; + private string $description = ''; - private QuantityFormatterDefinition $quantityFormatterDefinition; + private QuantityFormatterDefinition $quantityFormatterDefinition; - private Aggregate $aggregate; + private ?Aggregate $aggregate = null;🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 19-19: UndefinedClass
src/product/PriceTypeDefinition.php:19:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\Domain\Model\Unit\Unit does not exist (see https://psalm.dev/019)
[failure] 23-23: UndefinedClass
src/product/PriceTypeDefinition.php:23:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\formatter\QuantityFormatterDefinition does not exist (see https://psalm.dev/019)
71-77:⚠️ Potential issueImplement class existence validation
The TODO comment indicates missing validation for the formatter class.
Add validation to ensure the formatter class exists and implements the correct interface.
89-93:⚠️ Potential issueAdd validation in end() method
The method should validate that all required properties are set before returning.
Add validation for required properties:
public function end(): PriceTypeDefinitionCollection { - // Validate the PriceType and lock its state + if (!isset($this->unit)) { + throw new \RuntimeException('Unit is required but not set'); + } + if (empty($this->description)) { + throw new \RuntimeException('Description is required but not set'); + } + if (!isset($this->quantityFormatterDefinition)) { + throw new \RuntimeException('Quantity formatter is required but not set'); + } return $this->parent; }
🧹 Nitpick comments (1)
src/product/PriceTypeDefinition.php (1)
31-40: Add constructor documentation and clarify initializationThe constructor lacks PHPDoc documentation, and the purpose of the
init()method is unclear.Add constructor documentation and clarify the hook's purpose:
+ /** + * @param PriceTypeDefinitionCollection $parent Parent collection managing this definition + * @param TypeInterface $type Price type being defined + * @param TariffType $tariffType Associated tariff type + */ public function __construct( private readonly PriceTypeDefinitionCollection $parent, private readonly TypeInterface $type, TariffType $tariffType, ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/product/PriceTypeDefinition.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/PriceTypeDefinition.php
[failure] 19-19: UndefinedClass
src/product/PriceTypeDefinition.php:19:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\Domain\Model\Unit\Unit does not exist (see https://psalm.dev/019)
[failure] 23-23: UndefinedClass
src/product/PriceTypeDefinition.php:23:13: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\quantity\formatter\QuantityFormatterDefinition does not exist (see https://psalm.dev/019)
🔇 Additional comments (2)
src/product/PriceTypeDefinition.php (2)
100-103: Remove unused methodThe
measuredWithmethod is not used anywhere in the codebase and its implementation is a no-op.Remove this unused method.
137-139: Translate documentation to EnglishDocumentation should be in English for better maintainability and collaboration.
Replace the documentation with:
- * це параметер визначає агрегатну функцію яка застосовується для щоденно записаних ресурсів щоб визнизначти - * місячне споживання за яке потрібно пробілити клієнта + * This parameter defines the aggregate function that is applied to daily recorded resources + * to determine the monthly consumption for which the client needs to be billed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/product/TariffTypeDefinition.php (4)
17-21: Consider input validation in the constructor.
Currently, there is no validation of the$tariffTypeobject. If the constructor is a critical entry point for the system, you may wish to sanity-check or enforce constraints (e.g., ensuring$tariffTypeis fully formed).
35-39: Add validation or usage for$suggesterClass.
Currently, thesetPricesSuggester()method simply comments about validation but doesn’t apply it. Consider implementing validation or storing$suggesterClassif it’s intended for later usage. Otherwise, you may remove the method if it serves no purpose.
51-60: Improve readability & reduce complexity inhasBehavior().
The loop checks each$behavioragainstinstanceof $behaviorClassName. You may refactor using array helpers or short-circuit logic to make it more concise. For instance:return array_reduce( iterator_to_array($this->behaviorCollection), fn($carry, $behavior) => $carry || $behavior instanceof $behaviorClassName, false );
62-66: Clarify theend()method’s concerns.
If the goal is to "lock its state," consider throwing exceptions or performing final validations that ensureTariffTypeis correct before returning$this. Listing which validations occur may also help future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/product/TariffTypeDefinition.php(1 hunks)
🔇 Additional comments (2)
src/product/TariffTypeDefinition.php (2)
28-33: Potential null property usage.
The property$productis assigned inofProduct()but is never checked or validated for null outside this method. If other methods rely on$product, they might operate on an uninitialized property.Consider verifying if
$productis always set before usage.
9-10: Class implements an unshown interface.
The file mentionsParentNodeDefinitionInterface, but it is not shown. Please ensure that the interface is defined and accessible.Use the following script to confirm if the interface exists and is properly declared:
✅ Verification successful
Interface verification successful
TheParentNodeDefinitionInterfaceis properly defined insrc/product/ParentNodeDefinitionInterface.phpand is accessible within the same namespace.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the declaration of ParentNodeDefinitionInterface ast-grep --pattern $'interface ParentNodeDefinitionInterface {\n $$$\n}'Length of output: 72
Script:
#!/bin/bash # Search for any mentions of ParentNodeDefinitionInterface rg "ParentNodeDefinitionInterface" -A 2 # Get the namespace context and use statements from the implementing class cat src/product/TariffTypeDefinition.php | head -n 10Length of output: 1057
src/product/PriceTypeDefinition.php
Outdated
| use hiqdev\billing\registry\behavior\PriceTypeDefinitionBehaviourCollection; | ||
| use hiqdev\billing\registry\Domain\Model\TariffType; | ||
| use hiqdev\billing\registry\Domain\Model\Unit\Unit; | ||
| use hiqdev\billing\registry\invoice\InvoiceRepresentationCollection; | ||
| use hiqdev\billing\registry\product\Aggregate; | ||
| use hiqdev\billing\registry\quantity\formatter\QuantityFormatterDefinition; | ||
| use hiqdev\billing\registry\quantity\formatter\QuantityFormatterFactory; | ||
| use hiqdev\billing\registry\quantity\FractionQuantityData; | ||
| use hiqdev\billing\registry\Domain\Model\Unit\FractionUnit; |
There was a problem hiding this comment.
The same thing here.
We'll neet a call to figure it out.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/product/price/PriceTypeDefinition.php (1)
120-120:⚠️ Potential issueUndefined or external class reference.
You're referencing\hiqdev\billing\registry\measure\RcpTrafCollector, which static analysis flags as non-existent in this codebase. This likely causes runtime errors or relies on an unlisted external dependency, conflicting with your repository’s aim to avoid external packages.Consider removing or replacing the reference:
- public function measuredWith(\hiqdev\billing\registry\measure\RcpTrafCollector $param): self - { - return $this; - } + // Remove or replace with an internal interface or class + public function measuredWith(object $param): self + { + // ... + return $this; + }🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 120-120: UndefinedClass
src/product/price/PriceTypeDefinition.php:120:34: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\measure\RcpTrafCollector does not exist (see https://psalm.dev/019)src/product/TariffTypeDefinition.php (1)
12-12: 🛠️ Refactor suggestionMissing template parameter for
TariffTypeDefinitionInterface.
Psalm complains that you need a template parameter to fulfillTariffTypeDefinitionInterface's contract.Try adding the template annotation:
+/** + * @template TParent of TariffTypeDefinitionInterface + * @implements TariffTypeDefinitionInterface<TParent> + */ class TariffTypeDefinition implements TariffTypeDefinitionInterface {🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 12-12: MissingTemplateParam
src/product/TariffTypeDefinition.php:12:39: MissingTemplateParam: hiqdev\php\billing\product\TariffTypeDefinition has missing template params when extending hiqdev\php\billing\product\TariffTypeDefinitionInterface, expecting 1 (see https://psalm.dev/182)🪛 GitHub Actions: Psalm Static Analysis
[error] 12-12: MissingTemplateParam: hiqdev\php\billing\product\TariffTypeDefinition has missing template params when extending hiqdev\php\billing\product\TariffTypeDefinitionInterface, expecting 1 (see https://psalm.dev/182)
🧹 Nitpick comments (7)
src/product/Exception/AggregateNotDefinedException.php (1)
1-10: Good implementation, but contains unnecessary blank line.The class correctly extends the project's
LogicExceptionand follows PHP best practices. However, there's an unnecessary blank line inside the class body.class AggregateNotDefinedException extends LogicException { - }src/product/Exception/ProductNotDefinedException.php (1)
1-10: Good implementation, but contains unnecessary blank line.The class correctly extends the project's
LogicExceptionand follows PHP best practices. However, there's an unnecessary blank line inside the class body.class ProductNotDefinedException extends LogicException { - }src/product/Exception/TariffTypeLockedException.php (1)
5-8: Consider using project-specific LogicException for consistency.This class extends PHP's native
\LogicExceptionwhile other exception classes in this PR extend project-specific exceptions fromhiqdev\php\billing\Exception. Also, there's an unnecessary blank line in the class body.-use hiqdev\php\billing\Exception\LogicException; +use hiqdev\php\billing\Exception\LogicException; -class TariffTypeLockedException extends \LogicException +class TariffTypeLockedException extends LogicException { - }tests/unit/product/TariffTypeDefinitionTest.php (2)
54-60: Basic assertion forwithPrices().
You ensurewithPrices()returns the expectedPriceTypeDefinitionCollection. This is a good start, although you could add tests that verify price definitions in subsequent steps if needed.
62-68: Basic assertion forwithBehaviors().
You confirm the returned type isBehaviorTariffTypeCollection. As with prices, consider additional tests to validate specific behaviors if the logic grows more complex.src/product/BillingRegistry.php (1)
95-131: Behavior retrieval logic is thorough.
getBehavior()handles invalid class references and interface checks, then searches matching price types for the requested behavior. Re-usingfindBehaviorInPriceType()keeps the code organized.Consider unify or partially reuse logic from
getBehaviors()to avoid duplication of search patterns.src/product/TariffTypeDefinition.php (1)
62-68: Optional future extension forsetPricesSuggester().
Currently a placeholder for handling or storing a suggester class. If you rely on dynamic class usage, consider verifying it implements an appropriate interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/product/BillingRegistry.php(1 hunks)src/product/Exception/AggregateNotDefinedException.php(1 hunks)src/product/Exception/AggregateNotFoundException.php(1 hunks)src/product/Exception/ProductNotDefinedException.php(1 hunks)src/product/Exception/TariffTypeLockedException.php(1 hunks)src/product/TariffTypeDefinition.php(1 hunks)src/product/price/PriceTypeDefinition.php(1 hunks)tests/unit/product/TariffTypeDefinitionTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
src/product/BillingRegistry.php (1)
Learnt from: SilverFire
PR: hiqdev/php-billing#93
File: src/product/BillingRegistry.php:5-5
Timestamp: 2025-03-25T11:09:53.123Z
Learning: The `hiqdev/php-billing` repository should not depend on `advancedhosters/billing-registry`. Use internal interfaces and implementations instead.
src/product/TariffTypeDefinition.php (1)
Learnt from: SilverFire
PR: hiqdev/php-billing#93
File: src/product/BillingRegistry.php:5-5
Timestamp: 2025-03-25T11:09:53.123Z
Learning: The `hiqdev/php-billing` repository should not depend on `advancedhosters/billing-registry`. Use internal interfaces and implementations instead.
🧬 Code Definitions (3)
tests/unit/product/TariffTypeDefinitionTest.php (3)
src/product/Exception/ProductNotDefinedException.php (1)
ProductNotDefinedException(7-10)src/product/Exception/TariffTypeLockedException.php (1)
TariffTypeLockedException(5-8)src/product/TariffTypeDefinition.php (7)
TariffTypeDefinition(12-109)tariffType(28-31)ofProduct(33-39)getProduct(48-53)withPrices(70-75)withBehaviors(77-82)end(95-108)
src/product/BillingRegistry.php (10)
src/product/behavior/InvalidBehaviorException.php (1)
InvalidBehaviorException(7-9)src/product/Exception/AggregateNotFoundException.php (1)
AggregateNotFoundException(7-9)src/product/invoice/InvalidRepresentationException.php (1)
InvalidRepresentationException(7-10)src/product/price/PriceTypeDefinition.php (7)
PriceTypeDefinition(24-182)type(125-128)documentRepresentation(115-118)createQuantityFormatter(93-101)hasType(130-133)withBehaviors(140-143)getAggregate(174-181)src/product/quantity/QuantityFormatterNotFoundException.php (1)
QuantityFormatterNotFoundException(7-9)src/product/quantity/FractionQuantityData.php (1)
FractionQuantityData(7-14)src/product/behavior/BehaviorNotFoundException.php (1)
BehaviorNotFoundException(7-9)src/product/TariffTypeDefinition.php (3)
tariffType(28-31)withPrices(70-75)withBehaviors(77-82)src/product/BillingRegistryInterface.php (1)
priceTypes(13-13)src/product/ParentNodeDefinitionInterface.php (1)
withBehaviors(9-9)
src/product/TariffTypeDefinition.php (4)
src/product/behavior/BehaviorTariffTypeCollection.php (3)
BehaviorTariffTypeCollection(8-19)__construct(10-13)end(15-18)src/product/Exception/ProductNotDefinedException.php (1)
ProductNotDefinedException(7-10)src/product/Exception/TariffTypeLockedException.php (1)
TariffTypeLockedException(5-8)src/product/price/PriceTypeDefinitionCollection.php (4)
PriceTypeDefinitionCollection(13-57)__construct(19-26)end(48-51)count(53-56)
🪛 GitHub Check: PHP 8.3
src/product/TariffTypeDefinition.php
[failure] 12-12: MissingTemplateParam
src/product/TariffTypeDefinition.php:12:39: MissingTemplateParam: hiqdev\php\billing\product\TariffTypeDefinition has missing template params when extending hiqdev\php\billing\product\TariffTypeDefinitionInterface, expecting 1 (see https://psalm.dev/182)
src/product/price/PriceTypeDefinition.php
[failure] 120-120: UndefinedClass
src/product/price/PriceTypeDefinition.php:120:34: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\measure\RcpTrafCollector does not exist (see https://psalm.dev/019)
🪛 GitHub Actions: Psalm Static Analysis
src/product/TariffTypeDefinition.php
[error] 12-12: MissingTemplateParam: hiqdev\php\billing\product\TariffTypeDefinition has missing template params when extending hiqdev\php\billing\product\TariffTypeDefinitionInterface, expecting 1 (see https://psalm.dev/182)
🔇 Additional comments (17)
src/product/Exception/AggregateNotFoundException.php (1)
1-9: Good implementation of specialized exception class.The
AggregateNotFoundExceptionextends the project'sRuntimeExceptionand follows the PHP best practice of using strict typing. This class will be useful for specific error handling in theBillingRegistryclass.tests/unit/product/TariffTypeDefinitionTest.php (5)
16-24: Good coverage fortestTariffType()method.
Your test checks whether thetariffType()method returns the same instance ofTariffTypeInterfacethat was passed to the constructor. This confirms the contract.
26-33: Clear verification forofProduct()method.
This test correctly ensures thatofProduct()returns an instance ofTariffTypeDefinitionInterface, verifying the fluent interface promise.
35-43: Thorough check ofgetProduct()afterofProduct().
You confirm that the product is set and retrieved correctly, ensuring valid internal state.
45-52: Proper exception test for missing product.
ProductNotDefinedExceptionis tested, indicating reliable error handling ingetProduct().
70-82: Solid test for lock functionality.
testLockPreventsModification()correctly assertsTariffTypeLockedExceptionwhen attempting to modify a lockedTariffTypeDefinition. This strongly guards against unintended state changes.src/product/BillingRegistry.php (5)
38-45: Robust price types iteration.
YourpriceTypes()method correctly iterates over eachtariffTypeand yields theirPriceTypeDefinitionobjects. This approach is concise and clear.
47-73: Comprehensive check for representation class validity.
The method validates class existence and checks it implementsRepresentationInterface. This ensures correctness and prevents runtime type issues.
75-88: Clean solution forcreateQuantityFormatter().
You convert the string type to aTypeInterfaceand iterate through all price types to find a match, throwing aQuantityFormatterNotFoundExceptionif unavailable. This is well-structured.
146-163:getBehaviors()iterates both tariff types and price types.
This symmetrical approach matchesgetBehavior()logic. It's a powerful method for enumerating multiple behaviors.
165-176:getAggregate()clearly encapsulates the aggregator retrieval.
Your error handling withAggregateNotFoundExceptionensures consumers can distinguish missing aggregates from normal operation.src/product/TariffTypeDefinition.php (6)
33-39:ofProduct()approach is sound.
The method sets$productand returnsTariffTypeDefinitionInterface, enabling fluent usage.
48-53: SolidgetProduct()enforcement.
You verify$productis defined before returning, throwingProductNotDefinedExceptionif not. This enforces correct usage patterns.
70-75: Safe concurrency withwithPrices().
You ensure the definition is not locked before retrievingprices. This fosters immutability afterend().
77-82: Consistent pattern forwithBehaviors().
Mirrors thewithPrices()approach. Lock checks maintain consistent behavior.
84-93: Advanced check for behaviors.
hasBehavior()method iteratesbehaviorCollectionto find matching classes, facilitating flexible runtime expansions.
95-108: Well-structuredend()method.
You validate the product, ensure at least one price type, and lock further modifications. This finalizes the definition properly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/product/BillingRegistry.php (1)
147-164: 🛠️ Refactor suggestionAdd validation for behavior class in getBehaviors method.
The
getBehaviorsmethod should validate the behavior class similarly to how it's done in thegetBehaviormethod for consistency and to prevent errors.public function getBehaviors(string $behaviorClassWrapper): \Generator { + if (!class_exists($behaviorClassWrapper)) { + throw new InvalidBehaviorException( + sprintf('Behavior class "%s" does not exist', $behaviorClassWrapper) + ); + } + + if (!is_subclass_of($behaviorClassWrapper, BehaviorInterface::class)) { + throw new InvalidBehaviorException( + sprintf('Behavior class "%s" does not implement BehaviorInterface', $behaviorClassWrapper) + ); + } + foreach ($this->tariffTypes as $tariffType) {
🧹 Nitpick comments (7)
tests/unit/product/DummyBehavior.php (2)
9-14: Add type declarations for the context property and constructor parameterConsider adding appropriate type declarations to improve type safety.
- private $context; + private mixed $context; - public function __construct($context) + public function __construct(mixed $context)
16-19: Add return type declaration to getContext methodFor consistency with strict typing, add a return type declaration to the method signature.
- public function getContext() + public function getContext(): mixedsrc/product/Exception/BillingRegistryLockedException.php (1)
5-8: Consider adding a default error message to the exceptionAdding a descriptive default message would improve debugging and error handling.
class BillingRegistryLockedException extends \LogicException { + public function __construct(string $message = 'Cannot modify a locked billing registry', int $code = 0, \Throwable $previous = null) + { + parent::__construct($message, $code, $previous); + } }tests/unit/product/BillingRegistryTest.php (1)
85-89: Uncomment and implement the quantity formatter test.There's a commented-out test for quantity formatter exception handling. Consider implementing and enabling this test to ensure complete coverage of the
BillingRegistryclass functionality.-// public function testCreateQuantityFormatterThrowsExceptionWhenNotFound(): void -// { -// $this->expectException(QuantityFormatterNotFoundException::class); -// $this->registry->createQuantityFormatter('non-existent-type', $this->createMock(FractionQuantityData::class)); -// } + public function testCreateQuantityFormatterThrowsExceptionWhenNotFound(): void + { + $this->expectException(QuantityFormatterNotFoundException::class); + $this->registry->createQuantityFormatter('non-existent-type', $this->createMock(FractionQuantityData::class)); + }src/product/BillingRegistry.php (3)
76-89: Improve error message for quantity formatter not found.The error message when a quantity formatter is not found should include the type that wasn't found to help with debugging.
- throw new QuantityFormatterNotFoundException('Quantity formatter not found'); + throw new QuantityFormatterNotFoundException( + sprintf('Quantity formatter not found for type "%s"', $type) + );
134-145: Consider using interface instead of concrete class in parameter type.The
findBehaviorInPriceTypemethod uses a concretePriceTypeDefinitionclass as a parameter type, but using an interface would provide better abstraction and flexibility.- private function findBehaviorInPriceType( - PriceTypeDefinition $priceTypeDefinition, + private function findBehaviorInPriceType( + PriceTypeDefinitionInterface $priceTypeDefinition, string $behaviorClassWrapper ): ?BehaviorInterface {
166-177: Improve error message for aggregate not found.The error message when an aggregate is not found should include the type that wasn't found to help with debugging.
- throw new AggregateNotFoundException('Aggregate was not found'); + throw new AggregateNotFoundException( + sprintf('Aggregate not found for type "%s"', $type) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/product/BillingRegistry.php(1 hunks)src/product/Domain/Model/TariffTypeInterface.php(1 hunks)src/product/Exception/BillingRegistryLockedException.php(1 hunks)tests/unit/product/BillingRegistryTest.php(1 hunks)tests/unit/product/DummyBehavior.php(1 hunks)tests/unit/product/DummyTariffType.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/product/BillingRegistry.php (1)
Learnt from: SilverFire
PR: hiqdev/php-billing#93
File: src/product/BillingRegistry.php:5-5
Timestamp: 2025-03-25T11:09:53.123Z
Learning: The `hiqdev/php-billing` repository should not depend on `advancedhosters/billing-registry`. Use internal interfaces and implementations instead.
🧬 Code Definitions (4)
src/product/Domain/Model/TariffTypeInterface.php (1)
tests/unit/product/DummyTariffType.php (2)
name(8-11)label(13-16)
tests/unit/product/DummyTariffType.php (1)
src/product/Domain/Model/TariffTypeInterface.php (2)
name(7-7)label(9-9)
src/product/BillingRegistry.php (10)
src/product/behavior/InvalidBehaviorException.php (1)
InvalidBehaviorException(7-9)src/product/Exception/AggregateNotFoundException.php (1)
AggregateNotFoundException(7-9)src/product/Exception/BillingRegistryLockedException.php (1)
BillingRegistryLockedException(5-8)src/product/invoice/InvalidRepresentationException.php (1)
InvalidRepresentationException(7-10)src/product/price/PriceTypeDefinition.php (7)
PriceTypeDefinition(24-182)type(125-128)documentRepresentation(115-118)createQuantityFormatter(93-101)hasType(130-133)withBehaviors(140-143)getAggregate(174-181)src/product/quantity/QuantityFormatterNotFoundException.php (1)
QuantityFormatterNotFoundException(7-9)src/product/quantity/FractionQuantityData.php (1)
FractionQuantityData(7-14)src/product/behavior/BehaviorNotFoundException.php (1)
BehaviorNotFoundException(7-9)src/product/TariffTypeDefinition.php (2)
withPrices(70-75)withBehaviors(77-82)src/product/BillingRegistryInterface.php (1)
priceTypes(13-13)
tests/unit/product/BillingRegistryTest.php (5)
src/product/BillingRegistry.php (7)
BillingRegistry(19-178)addTariffType(25-32)priceTypes(39-46)lock(34-37)getRepresentationsByType(52-74)getAggregate(166-177)getBehavior(103-132)src/product/Exception/BillingRegistryLockedException.php (1)
BillingRegistryLockedException(5-8)tests/unit/product/DummyTariffType.php (2)
DummyTariffType(7-17)name(8-11)tests/unit/product/DummyBehavior.php (2)
DummyBehavior(7-20)getContext(16-19)src/product/Domain/Model/TariffTypeInterface.php (1)
name(7-7)
🔇 Additional comments (15)
src/product/Domain/Model/TariffTypeInterface.php (1)
1-10: Well-designed interface with clear purposeThe interface follows good design principles with two focused methods that represent the core identity of a tariff type. The use of return type declarations for strict typing is appropriate, and the naming convention clearly indicates the purpose of each method.
tests/unit/product/DummyTariffType.php (1)
7-17: Clean implementation of TariffTypeInterface for testing purposesThe class properly implements the interface methods with appropriate return types and values for testing. No issues found.
tests/unit/product/BillingRegistryTest.php (7)
1-16: LGTM! - Well-structured imports and namespace.The necessary imports for the test class are properly organized, including the appropriate exceptions and interfaces required for testing.
17-27: LGTM! - Good test class structure with appropriate setup.The test class is well-structured with properly typed properties and a clean setup method that initializes the registry and a tariff type definition for the tests.
29-42: LGTM! - Effective test for core registry functionality.The test validates that a tariff type can be added to the registry and its price types can be retrieved correctly, with appropriate assertions to verify the count and identity of the retrieved types.
44-51: LGTM! - Good exception testing for locked registry.This test properly validates that attempting to modify a locked registry will throw the expected
BillingRegistryLockedException.
53-57: LGTM! - Proper exception testing for invalid representation classes.The test correctly verifies that attempting to get representations for an invalid class throws the expected
InvalidRepresentationException.
59-63: LGTM! - Properly testing aggregate not found scenario.The test correctly validates that attempting to retrieve a non-existent aggregate type throws the expected
AggregateNotFoundException.
79-83: LGTM! - Good exception testing for behavior not found.This test correctly verifies that attempting to retrieve a non-existent behavior throws the expected
BehaviorNotFoundException.src/product/BillingRegistry.php (6)
1-18: LGTM! - Good namespace and imports.The class uses the appropriate namespace and imports the required dependencies. The implementation follows the learnings provided about not depending on external repositories.
19-32: LGTM! - Clean implementation of registry core functionality.The class declaration and the
addTariffTypemethod are well implemented with proper type hints and exception handling when trying to modify a locked registry.
34-46: LGTM! - Good implementation of lock and priceTypes methods.The
lockmethod is simple and effective, and thepriceTypesmethod correctly iterates through the tariff types and their prices, yielding a generator of price type definitions.
63-74: LGTM! - Good implementation of representation gathering.The method correctly iterates through price types and collects representations of the specified type.
91-94: LGTM! - Good utility method for type conversion.The
convertStringTypeToTypemethod properly uses theType::anyId()factory method to convert string types.
96-132: LGTM! - Well-implemented behavior retrieval with good validation.The
getBehaviormethod has thorough validation of the behavior class and provides clear error messages for different failure scenarios.
| /** | ||
| * @param string $representationClass | ||
| * @return RepresentationInterface[] | ||
| */ | ||
| public function getRepresentationsByType(string $representationClass): array | ||
| { | ||
| if (!class_exists($representationClass)) { | ||
| throw new InvalidRepresentationException("Class '$representationClass' does not exist"); | ||
| } | ||
|
|
||
| if (!is_subclass_of($representationClass, RepresentationInterface::class)) { | ||
| throw new InvalidBehaviorException( | ||
| sprintf('Representation class "%s" does not implement RepresentationInterface', $representationClass) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Fix the exception type in getRepresentationsByType.
An incorrect exception type is thrown when validating if the class implements RepresentationInterface. It should throw InvalidRepresentationException to be consistent with the validation pattern.
- throw new InvalidBehaviorException(
+ throw new InvalidRepresentationException(
sprintf('Representation class "%s" does not implement RepresentationInterface', $representationClass)
);📝 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.
| /** | |
| * @param string $representationClass | |
| * @return RepresentationInterface[] | |
| */ | |
| public function getRepresentationsByType(string $representationClass): array | |
| { | |
| if (!class_exists($representationClass)) { | |
| throw new InvalidRepresentationException("Class '$representationClass' does not exist"); | |
| } | |
| if (!is_subclass_of($representationClass, RepresentationInterface::class)) { | |
| throw new InvalidBehaviorException( | |
| sprintf('Representation class "%s" does not implement RepresentationInterface', $representationClass) | |
| ); | |
| } | |
| /** | |
| * @param string $representationClass | |
| * @return RepresentationInterface[] | |
| */ | |
| public function getRepresentationsByType(string $representationClass): array | |
| { | |
| if (!class_exists($representationClass)) { | |
| throw new InvalidRepresentationException("Class '$representationClass' does not exist"); | |
| } | |
| if (!is_subclass_of($representationClass, RepresentationInterface::class)) { | |
| throw new InvalidRepresentationException( | |
| sprintf('Representation class "%s" does not implement RepresentationInterface', $representationClass) | |
| ); | |
| } | |
| // Rest of the method implementation... | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/product/BillingRegistryTest.php (1)
85-89: Consider documenting why the test is commented out.The commented-out test for quantity formatter might indicate functionality that's not yet implemented or is still in development. Consider adding a TODO comment explaining why it's commented out and when it should be enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/product/BillingRegistryTest.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/unit/product/BillingRegistryTest.php (5)
src/product/BillingRegistry.php (7)
BillingRegistry(19-178)addTariffType(25-32)priceTypes(39-46)lock(34-37)getRepresentationsByType(52-74)getAggregate(166-177)getBehavior(103-132)src/product/Exception/BillingRegistryLockedException.php (1)
BillingRegistryLockedException(5-8)tests/unit/product/DummyTariffType.php (2)
DummyTariffType(7-17)name(8-11)tests/unit/product/DummyBehavior.php (2)
DummyBehavior(7-20)getContext(16-19)src/product/Domain/Model/TariffTypeInterface.php (1)
name(7-7)
🔇 Additional comments (4)
tests/unit/product/BillingRegistryTest.php (4)
1-16: Well-structured imports and namespace declaration.The file has a proper namespace declaration and imports all necessary classes. Using strict typing is a good practice for enforcing type safety in PHP.
17-27: Good test class structure and setup.The test class is properly structured with typed properties and a correctly implemented setUp method that initializes the test dependencies before each test runs.
29-42: LGTM: Effective test for adding and retrieving tariff types.The test properly verifies that a tariff type can be added to the registry and retrieved through the priceTypes() method. It follows the AAA pattern (Arrange-Act-Assert) and includes appropriate assertions.
44-83: Well-implemented test cases covering both happy paths and error scenarios.The tests effectively cover:
- Registry locking functionality
- Error handling for invalid representations, aggregates, and behaviors
- Behavior attachment and retrieval
The implementation in
testGetBehaviorcorrectly creates a new tariff type definition with behavior and properly adds this same variable to the registry, addressing the issue mentioned in previous review comments.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/product/BillingRegistry.php (3)
58-62:⚠️ Potential issueWrong exception type when validating representation class.
When validating if a class implements
RepresentationInterface, you're throwingInvalidBehaviorExceptioninstead ofInvalidRepresentationException.if (!is_subclass_of($representationClass, RepresentationInterface::class)) { - throw new InvalidBehaviorException( + throw new InvalidRepresentationException( sprintf('Representation class "%s" does not implement RepresentationInterface', $representationClass) ); }
147-164: 🛠️ Refactor suggestionAdd validation for behavior class in getBehaviors method.
Unlike the
getBehaviormethod,getBehaviorsdoesn't validate if$behaviorClassWrapperexists or implementsBehaviorInterface. This could lead to runtime errors if an invalid class is provided.public function getBehaviors(string $behaviorClassWrapper): \Generator { + if (!class_exists($behaviorClassWrapper)) { + throw new InvalidBehaviorException( + sprintf('Behavior class "%s" does not exist', $behaviorClassWrapper) + ); + } + + if (!is_subclass_of($behaviorClassWrapper, BehaviorInterface::class)) { + throw new InvalidBehaviorException( + sprintf('Behavior class "%s" does not implement BehaviorInterface', $behaviorClassWrapper) + ); + } + foreach ($this->tariffTypeDefinitions as $tariffType) { foreach ($tariffType->withBehaviors() as $behavior) { if ($behavior instanceof $behaviorClassWrapper) {
1-18:⚠️ Potential issueMissing import for AggregateInterface.
The
getAggregatemethod returnsAggregateInterfacebut there's no import for this interface.Add the missing import:
use hiqdev\php\billing\product\behavior\BehaviorNotFoundException; use hiqdev\php\billing\type\Type; use hiqdev\php\billing\type\TypeInterface; +use hiqdev\php\billing\product\AggregateInterface;
🧹 Nitpick comments (7)
tests/unit/product/BillingRegistryTest.php (2)
71-71: Remove unnecessary comment.The comment
// monthly()doesn't add any value and appears to be leftover from earlier development.- $this->registry->addTariffType($tariffTypeDefinition); // monthly() + $this->registry->addTariffType($tariffTypeDefinition);
84-88: Consider uncommenting this test.This commented-out test for
createQuantityFormatterwould provide better test coverage for theBillingRegistryclass.If the method is fully implemented, uncomment and complete this test to ensure the quantity formatter exception is properly thrown when needed.
src/product/BillingRegistry.php (5)
48-51: DocBlock return type should match implementation.The DocBlock specifies
@return RepresentationInterface[]but the method actually returns an array of instances of the specified representation class./** * @param string $representationClass - * @return RepresentationInterface[] + * @return array<int, RepresentationInterface> Array of representations of the specified class */
88-88: Include type information in error message.The error message for
QuantityFormatterNotFoundExceptiondoesn't include the type that wasn't found, which makes debugging harder.-throw new QuantityFormatterNotFoundException('Quantity formatter not found'); +throw new QuantityFormatterNotFoundException( + sprintf('Quantity formatter not found for type "%s"', $type) +);
176-176: Include type information in error message for AggregateNotFoundException.Similar to the
QuantityFormatterNotFoundException, the error message forAggregateNotFoundExceptiondoesn't include the type that wasn't found.-throw new AggregateNotFoundException('Aggregate was not found'); +throw new AggregateNotFoundException( + sprintf('Aggregate not found for type "%s"', $type) +);
147-147: Add DocBlock for getBehaviors method.The
getBehaviorsmethod is missing a DocBlock describing its purpose, parameters, and return type.+/** + * Retrieves all behaviors of a specific class type from tariff type definitions and price type definitions. + * + * @param string $behaviorClassWrapper The behavior class to match + * @return \Generator<int, BehaviorInterface> Generator yielding behaviors of the specified class + * @throws InvalidBehaviorException When behavior class is invalid + */ public function getBehaviors(string $behaviorClassWrapper): \Generator
166-166: Add DocBlock for getAggregate method.The
getAggregatemethod is missing a DocBlock describing its purpose, parameters, and return type.+/** + * Retrieves an aggregate for a given type. + * + * @param string $type The type to find an aggregate for + * @return AggregateInterface The aggregate for the given type + * @throws AggregateNotFoundException When no aggregate is found for the given type + */ public function getAggregate(string $type): AggregateInterface
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/product/BillingRegistry.php(1 hunks)tests/unit/product/BillingRegistryTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/product/BillingRegistry.php (1)
Learnt from: SilverFire
PR: hiqdev/php-billing#93
File: src/product/BillingRegistry.php:5-5
Timestamp: 2025-03-27T02:39:39.583Z
Learning: The `hiqdev/php-billing` repository should not depend on `advancedhosters/billing-registry`. Use internal interfaces and implementations instead.
🧬 Code Definitions (2)
tests/unit/product/BillingRegistryTest.php (5)
src/product/BillingRegistry.php (7)
BillingRegistry(19-178)addTariffType(25-32)priceTypes(39-46)lock(34-37)getRepresentationsByType(52-74)getAggregate(166-177)getBehavior(103-132)src/product/Exception/BillingRegistryLockedException.php (1)
BillingRegistryLockedException(5-8)tests/unit/product/DummyTariffType.php (2)
DummyTariffType(7-17)name(8-11)tests/unit/product/DummyBehavior.php (2)
DummyBehavior(7-20)getContext(16-19)src/product/Domain/Model/TariffTypeInterface.php (1)
name(7-7)
src/product/BillingRegistry.php (10)
src/product/behavior/InvalidBehaviorException.php (1)
InvalidBehaviorException(7-9)src/product/Exception/AggregateNotFoundException.php (1)
AggregateNotFoundException(7-9)src/product/Exception/BillingRegistryLockedException.php (1)
BillingRegistryLockedException(5-8)src/product/invoice/InvalidRepresentationException.php (1)
InvalidRepresentationException(7-10)src/product/price/PriceTypeDefinition.php (4)
PriceTypeDefinition(24-182)type(125-128)createQuantityFormatter(93-101)getAggregate(174-181)src/product/quantity/QuantityFormatterNotFoundException.php (1)
QuantityFormatterNotFoundException(7-9)src/product/quantity/FractionQuantityData.php (1)
FractionQuantityData(7-14)src/product/behavior/BehaviorNotFoundException.php (1)
BehaviorNotFoundException(7-9)src/product/BillingRegistryInterface.php (1)
priceTypes(13-13)src/product/TariffTypeDefinition.php (1)
withPrices(70-75)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/unit/product/BillingRegistryTest.php (3)
89-93: Commented-out test code should be completed or removed.There is a commented-out test method
testCreateQuantityFormatterThrowsExceptionWhenNotFound()which suggests incomplete functionality. Additionally, it referencesFractionQuantityDataclass which is not imported.Either:
- Complete and uncomment the test, adding the missing import:
use hiqdev\php\billing\product\quantity\FractionQuantityData;
- Or remove the commented-out code if the functionality is not implemented yet.
29-42: Verify test coverage for complex operations.This test verifies basic operations for adding a tariff type and retrieving price types, but doesn't test edge cases like adding multiple tariff types or handling duplicate tariff types.
Consider expanding test coverage with additional scenarios:
public function testAddMultipleTariffTypes(): void { $type1 = Type::anyId('dummy1'); $type2 = Type::anyId('dummy2'); $tariffType1 = new DummyTariffType(); $tariffType2 = new DummyTariffType2(); // Create an additional dummy class $tariffTypeDefinition1 = new TariffTypeDefinition($tariffType1); $tariffTypeDefinition2 = new TariffTypeDefinition($tariffType2); $tariffTypeDefinition1->withPrices()->priceType($type1); $tariffTypeDefinition2->withPrices()->priceType($type2); $this->registry->addTariffType($tariffTypeDefinition1); $this->registry->addTariffType($tariffTypeDefinition2); $priceTypes = iterator_to_array($this->registry->priceTypes()); $this->assertCount(2, $priceTypes); }
67-81: Behavior retrieval should be tested with multiple behaviors.The current test only verifies retrieving a single behavior. To ensure the registry correctly handles multiple behaviors and behavior type matching, consider testing with multiple behaviors of different types.
Enhance the behavior test:
public function testGetBehaviorWithMultipleBehaviorTypes(): void { $tariffType = new DummyTariffType(); $tariffTypeDefinition = new TariffTypeDefinition($tariffType); $dummyBehavior = new DummyBehavior('dummy'); $otherBehavior = new OtherBehavior(); // Create another behavior implementation $tariffTypeDefinition ->withPrices() ->priceType(Type::anyId('dummy')) ->withBehaviors() ->attach($dummyBehavior) ->attach($otherBehavior); $this->registry->addTariffType($tariffTypeDefinition); $behavior1 = $this->registry->getBehavior($tariffType->name(), DummyBehavior::class); $behavior2 = $this->registry->getBehavior($tariffType->name(), OtherBehavior::class); $this->assertSame($dummyBehavior->getContext(), $behavior1->getContext()); $this->assertInstanceOf(OtherBehavior::class, $behavior2); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/product/BillingRegistryTest.php(1 hunks)
🔇 Additional comments (1)
tests/unit/product/BillingRegistryTest.php (1)
38-41: Test assertion likely incorrect: Mismatch between generator output and test expectation.According to a previous review comment, the implementation of
BillingRegistry::priceTypes()yields price type definitions (not tariff type definitions). However, the assertion is checking if$priceTypes[0]is the same as$this->tariffTypeDefinition, which is a tariff type definition, not a price type definition.
| <?php declare(strict_types=1); | ||
|
|
||
| namespace hiqdev\php\billing\tests\unit\product; | ||
|
|
||
| use hiqdev\php\billing\product\BillingRegistry; | ||
| use hiqdev\php\billing\product\Exception\BillingRegistryLockedException; | ||
| use hiqdev\php\billing\product\TariffTypeDefinition; | ||
| use hiqdev\php\billing\product\TariffTypeDefinitionInterface; | ||
| use hiqdev\php\billing\product\Exception\AggregateNotFoundException; | ||
| use hiqdev\php\billing\product\invoice\InvalidRepresentationException; | ||
| use hiqdev\php\billing\product\quantity\QuantityFormatterNotFoundException; | ||
| use hiqdev\php\billing\product\behavior\BehaviorInterface; | ||
| use hiqdev\php\billing\product\behavior\BehaviorNotFoundException; | ||
| use hiqdev\php\billing\type\Type; | ||
| use PHPUnit\Framework\TestCase; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing definition for dummy test classes.
The test uses DummyTariffType and DummyBehavior classes (lines 26, 67, 69) but they are not defined in this file and there are no imports for them.
Add the missing test class definitions to complete the test. You can define them at the end of this file:
class DummyTariffType
{
public function name(): string
{
return 'dummy';
}
}
class DummyBehavior implements BehaviorInterface
{
private string $context;
public function __construct(string $context)
{
$this->context = $context;
}
public function getContext(): string
{
return $this->context;
}
}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/unit/product/BillingRegistryTest.php (2)
1-16:⚠️ Potential issueMissing definition or import for DummyBehavior class.
The test is using
DummyBehaviorin multiple places (specifically lines 69, 79, 86) but there is no import statement for this class and it's not defined within the file. WhileDummyTariffTypeis defined in a separate file (as shown in the relevant code snippets), I don't see a definition forDummyBehavior.Either:
- Import the class if it exists elsewhere:
use hiqdev\php\billing\tests\unit\product\DummyBehavior;- Or define it in this file:
class DummyBehavior implements BehaviorInterface { private string $context; public function __construct(string $context) { $this->context = $context; } public function getContext(): string { return $this->context; } }
38-41:⚠️ Potential issueMismatch between test expectation and registry implementation.
There's an inconsistency between what the test expects and what the
BillingRegistry::priceTypes()method actually returns. Based on the past review comments and looking at the implementation, the method yields price type definitions from each tariff type definition'swithPrices()method, not the tariff type definitions themselves.Either:
- Update the test expectation to check for a price type definition, or
- Update the
priceTypes()method to yield tariff type definitions insteadVerify which option is correct based on the intended behavior of the
BillingRegistryclass.
🧹 Nitpick comments (1)
tests/unit/product/BillingRegistryTest.php (1)
89-93: Clarify the status of the commented-out test.This test is commented out, which suggests incomplete functionality or a test that's not ready to be run. Add a comment explaining why this test is disabled (e.g., "TODO: Enable when quantity formatter functionality is implemented") to provide context for other developers.
-// public function testCreateQuantityFormatterThrowsExceptionWhenNotFound(): void -// { -// $this->expectException(QuantityFormatterNotFoundException::class); -// $this->registry->createQuantityFormatter('non-existent-type', $this->createMock(FractionQuantityData::class)); -// } +// TODO: Enable when quantity formatter functionality is implemented +// public function testCreateQuantityFormatterThrowsExceptionWhenNotFound(): void +// { +// $this->expectException(QuantityFormatterNotFoundException::class); +// $this->registry->createQuantityFormatter('non-existent-type', $this->createMock(FractionQuantityData::class)); +// }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/product/BillingRegistryTest.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/unit/product/BillingRegistryTest.php (3)
src/product/BillingRegistry.php (5)
BillingRegistry(19-178)addTariffType(25-32)priceTypes(39-46)lock(34-37)getBehavior(103-132)src/product/Exception/BillingRegistryLockedException.php (1)
BillingRegistryLockedException(5-8)tests/unit/product/DummyTariffType.php (2)
DummyTariffType(7-17)name(8-11)
🔇 Additional comments (5)
tests/unit/product/BillingRegistryTest.php (5)
65-81: Test is using correct behavior testing pattern.The test correctly creates a tariff type definition with a behavior attached, adds it to the registry, and then verifies the behavior can be retrieved with the correct context. This pattern ensures the behavior association and retrieval functionality works as expected.
44-51: Registry locking test is well-implemented.This test correctly verifies that the registry cannot be modified after it has been locked, by expecting a
BillingRegistryLockedExceptionwhen attempting to add a tariff type to a locked registry. This ensures the locking mechanism functions properly.
53-57: Exception test for invalid representation class is concise and effective.The test correctly checks that the
getRepresentationsByTypemethod throws anInvalidRepresentationExceptionwhen given an invalid class name. This ensures proper validation of representation classes.
59-63: Test for aggregate not found exception is properly implemented.This test correctly verifies that the
getAggregatemethod throws anAggregateNotFoundExceptionwhen requesting a non-existent aggregate type. This ensures the method properly handles missing aggregates.
83-87: Behavior not found exception test is correctly structured.This test correctly verifies that the
getBehaviormethod throws aBehaviorNotFoundExceptionwhen requesting a non-existent behavior type. This ensures proper exception handling for missing behaviors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/unit/product/BillingRegistryTest.php (5)
16-26: Good test class structure with proper setup.The test class is well defined with private properties and a proper
setUp()method that initializes test objects before each test.However, there's no import for
DummyTariffTypeused in line 25. While it appears to be defined in a separate file per the relevant code snippets, it would be clearer to add an import statement.+use hiqdev\php\billing\tests\unit\product\DummyTariffType;
55-59: Test for invalid representation class is correctly implemented.The test verifies that attempting to get representations by a non-existent class throws an
InvalidRepresentationException.However, consider adding a test for the success case of
getRepresentationsByTypeto complete the test coverage.
61-65: Test for aggregate not found is correctly implemented.The test verifies that attempting to get an aggregate for a non-existent type throws an
AggregateNotFoundException.Consider adding a test for the success case of
getAggregateto complete the test coverage.
67-83: Test for behavior retrieval is correctly implemented.The test properly creates a tariff type with a behavior and verifies that the behavior can be retrieved from the registry.
However, there's no import for
DummyBehaviorused in line 71. While it appears to be defined in a separate file per the relevant code snippets, it would be clearer to add an import statement.+use hiqdev\php\billing\tests\unit\product\DummyBehavior;
91-95: Commented-out test for quantity formatter.This commented-out test suggests planned functionality for testing quantity formatter creation. It would be helpful to either:
- Implement and uncomment the test if the functionality exists
- Add a comment explaining why it's commented out or create a TODO task
- Remove it if it's no longer relevant
Additionally, the test references
QuantityFormatterNotFoundExceptionandFractionQuantityDatawhich aren't imported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/product/BillingRegistryTest.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/unit/product/BillingRegistryTest.php (5)
src/product/BillingRegistry.php (7)
BillingRegistry(19-178)addTariffType(25-32)priceTypes(39-46)lock(34-37)getRepresentationsByType(52-74)getAggregate(166-177)getBehavior(103-132)src/product/Exception/BillingRegistryLockedException.php (1)
BillingRegistryLockedException(5-8)tests/unit/product/DummyTariffType.php (2)
DummyTariffType(7-17)name(8-11)tests/unit/product/DummyBehavior.php (2)
DummyBehavior(7-20)getContext(16-19)src/product/Domain/Model/TariffTypeInterface.php (1)
name(7-7)
🔇 Additional comments (4)
tests/unit/product/BillingRegistryTest.php (4)
1-15: Well-structured imports and namespace declaration.The file has proper namespace declaration and imports all necessary dependencies.
28-44: Test for adding tariff type and retrieving price types is correctly implemented.The test verifies that a tariff type with a price type can be added to the registry and then correctly retrieved.
46-53: Test for registry lock functionality is correctly implemented.The test properly verifies that once the registry is locked, attempting to add a tariff type throws a
BillingRegistryLockedException.
85-89: Test for behavior not found is correctly implemented.The test verifies that attempting to get a behavior for a non-existent type throws a
BehaviorNotFoundException.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unit/product/BillingRegistryTest.php (3)
92-96: Implement or remove the commented-out test.This commented-out test method (
testCreateQuantityFormatterThrowsExceptionWhenNotFound) suggests there's functionality around quantity formatters that should be tested. Either implement this test if the functionality exists, or remove it if it's for future development to avoid confusion.
5-14: Consider organizing imports alphabetically.For better maintainability and readability, consider organizing the import statements alphabetically. This makes it easier to scan for specific imports and identify potential duplicates.
1-97: Overall test suite looks good but lacks docblocks.The test suite provides good coverage of the BillingRegistry functionality with clear test methods. Consider adding PHPDoc comments to test methods explaining what each test is verifying for better documentation.
+ /** + * Tests adding a tariff type and verifying the retrieved price types + */ public function testAddTariffTypeAndRetrievePriceTypes(): void { // ... } + /** + * Tests that the registry cannot be modified after being locked + */ public function testLockPreventsModification(): void { // ... } // Add similar docblocks for other test methods
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/product/BillingRegistryTest.php(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/unit/product/BillingRegistryTest.php (4)
src/product/BillingRegistry.php (5)
BillingRegistry(19-178)addTariffType(25-32)priceTypes(39-46)lock(34-37)getBehavior(103-132)src/product/Exception/BillingRegistryLockedException.php (1)
BillingRegistryLockedException(5-8)tests/unit/product/DummyTariffType.php (1)
DummyTariffType(7-17)tests/unit/product/DummyBehavior.php (2)
DummyBehavior(7-20)getContext(16-19)
🔇 Additional comments (2)
tests/unit/product/BillingRegistryTest.php (2)
67-84: The behavior test implementation looks good.I can see that you've properly addressed the previous issue where the test was adding
$this->tariffTypeDefinitioninstead of the newly created definition with behavior. The test now correctly adds the$tariffTypeDefinitionwith the attached behavior to the registry.
28-44: The price types test implementation is correct.The test now properly asserts against the price type definition's type rather than comparing against the tariff type definition itself, addressing the previous mismatch between generator output and test expectation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/product/behavior/Behavior.php (2)
1-20: Good implementation with minor parameter naming inconsistencyThe abstract Behavior class implementation looks good overall with appropriate type hints and return types. However, there's a small inconsistency in the parameter naming for
setTariffType().The parameter is named
$tariffTypeNamebut it's actually aTariffTypeInterfaceobject, not just a name. Consider renaming it to$tariffTypeto match the property name for better consistency.- public function setTariffType(TariffTypeInterface $tariffTypeName): void + public function setTariffType(TariffTypeInterface $tariffType): void { - $this->tariffType = $tariffTypeName; + $this->tariffType = $tariffType; }
9-9: Consider initializing $tariffType or handling null stateThe
$tariffTypeproperty is never initialized in a constructor, which could lead to an error ifgetTariffType()is called beforesetTariffType(). Consider either:
- Initializing the property in a constructor, or
- Making the property nullable and handling potential null values in
getTariffType()- private TariffTypeInterface $tariffType; + private ?TariffTypeInterface $tariffType = null; public function getTariffType(): TariffTypeInterface { + if ($this->tariffType === null) { + throw new \RuntimeException('TariffType is not set'); + } return $this->tariffType; }tests/unit/product/behavior/TestBehavior.php (2)
9-19: Add type hints for $context property and methodFor consistency with strict types declaration and better code quality, consider adding appropriate type hints for the
$contextproperty and thegetContext()method. This will improve code maintainability and help catch type-related errors earlier.- private $context; + private mixed $context; - public function __construct($context) + public function __construct(mixed $context) { $this->context = $context; } - public function getContext() + public function getContext(): mixed { return $this->context; }If you know the expected type of
$context, use that specific type instead ofmixed.
11-14: Consider calling parent constructor if neededIf the parent
Behaviorclass has or might have a constructor in the future, consider callingparent::__construct()to ensure proper initialization.public function __construct($context) { + parent::__construct(); $this->context = $context; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/product/behavior/Behavior.php(1 hunks)tests/unit/product/BillingRegistryTest.php(1 hunks)tests/unit/product/behavior/FakeBehavior.php(1 hunks)tests/unit/product/behavior/TestBehavior.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/product/behavior/FakeBehavior.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/product/BillingRegistryTest.php
🔇 Additional comments (1)
tests/unit/product/behavior/TestBehavior.php (1)
7-20:❓ Verification inconclusive
Verify abstract class implementation
Ensure that this test class implements any abstract methods that might be required by the parent
Behaviorclass orBehaviorInterface. The file doesn't show any abstract method implementation, so it's worth verifying that all required methods are implemented.
🏁 Script executed:
#!/bin/bash # Check if Behavior class or BehaviorInterface has any abstract methods that need implementation echo "Checking for abstract methods in Behavior class or BehaviorInterface:" rg -A 3 "abstract function|function\s+[a-zA-Z0-9_]+\s*\(.*\)\s*;" src/product/behavior/BehaviorInterface.php src/product/behavior/Behavior.phpLength of output: 367
Abstract Method Implementation Verified – Please Confirm Base Class Location
After re‑examining the codebase, no abstract methods were found in theBehaviorInterfaceand no implementation details related to abstract requirements were detected. The absence of output regarding abstract methods in our search (aside from an error aboutsrc/product/behavior/Behavior.phpnot being found) suggests that theTestBehaviorclass does not need to implement any additional methods. However, please double‑check that the intended base class (or its equivalent) is correctly located and referenced in the codebase.
- Confirm that the
Behaviorclass isn’t defined elsewhere if it should exist, or that its omission is intentional.- Verify that the
BehaviorInterfacedoes not require explicit implementations beyond what is already present in the test class.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/product/BillingRegistryTest.php (1)
171-175: Consider implementing the commented-out test.There's a commented-out test for quantity formatter validation. Consider implementing this test if the functionality exists, or add a TODO comment explaining why it's commented out if the feature is planned for future implementation.
-// public function testCreateQuantityFormatterThrowsExceptionWhenNotFound(): void -// { -// $this->expectException(QuantityFormatterNotFoundException::class); -// $this->registry->createQuantityFormatter('non-existent-type', $this->createMock(FractionQuantityData::class)); -// } +// TODO: Implement this test when QuantityFormatter functionality is added +// public function testCreateQuantityFormatterThrowsExceptionWhenNotFound(): void +// { +// $this->expectException(QuantityFormatterNotFoundException::class); +// $this->registry->createQuantityFormatter('non-existent-type', $this->createMock(FractionQuantityData::class)); +// }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/unit/product/BillingRegistryTest.php(1 hunks)tests/unit/product/Domain/Model/DummyTariffType.php(1 hunks)tests/unit/product/Domain/Model/FakeTariffType.php(1 hunks)tests/unit/product/behavior/FakeBehavior.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/product/behavior/FakeBehavior.php
🧰 Additional context used
🧬 Code Definitions (3)
tests/unit/product/Domain/Model/FakeTariffType.php (1)
tests/unit/product/Domain/Model/DummyTariffType.php (2)
name(9-12)label(14-17)
tests/unit/product/Domain/Model/DummyTariffType.php (1)
tests/unit/product/Domain/Model/FakeTariffType.php (2)
name(9-12)label(14-17)
tests/unit/product/BillingRegistryTest.php (5)
src/product/BillingRegistry.php (5)
BillingRegistry(19-178)addTariffType(25-32)priceTypes(39-46)lock(34-37)getBehavior(103-132)src/product/Exception/BillingRegistryLockedException.php (1)
BillingRegistryLockedException(5-8)tests/unit/product/behavior/TestBehavior.php (2)
TestBehavior(7-20)getContext(16-19)tests/unit/product/Domain/Model/DummyTariffType.php (1)
DummyTariffType(7-18)tests/unit/product/Domain/Model/FakeTariffType.php (1)
FakeTariffType(7-18)
🔇 Additional comments (9)
tests/unit/product/Domain/Model/DummyTariffType.php (1)
1-18: Looks good - implementation follows test double pattern correctlyThe implementation of
DummyTariffTypeis straightforward and follows the same pattern as the existingFakeTariffType. It correctly implements theTariffTypeInterfacewith simple string returns for both required methods. The class is properly namespaced and uses strict typing, which is good practice.tests/unit/product/Domain/Model/FakeTariffType.php (1)
1-18: The implementation is concise and follows the established pattern.The
FakeTariffTypeclass correctly implements theTariffTypeInterfacewith the required methodsname()andlabel(), returning appropriate values similar to the pattern established inDummyTariffType. This test helper will be useful for creating mock tariff types in your unit tests.tests/unit/product/BillingRegistryTest.php (7)
20-48: Test structure and assertions look good.The test method properly demonstrates adding a tariff type to the registry and then verifying that the price types can be retrieved correctly. The assertions check both the count and the type value.
50-57: Lock functionality test is correct.This test properly verifies that the registry cannot be modified after being locked, by expecting the appropriate exception.
59-69: Error case tests are properly implemented.Both tests verify that the appropriate exceptions are thrown when invalid inputs are provided to the methods
getRepresentationsByTypeandgetAggregate.
71-88: Behavior retrieval test is well structured.The test correctly sets up a tariff type with a behavior, adds it to the registry, and then verifies that the behavior can be retrieved with the correct context.
90-125: Comprehensive test for behavior retrieval with multiple tariff type definitions.This test correctly verifies multiple scenarios:
- Retrieving a behavior for a specific type
- Retrieving different behaviors from the same tariff type definition
- Verifying behavior context values match expectations
The test uses a clean, fluent builder pattern to set up the test scenario.
127-163: Test for behavior retrieval across multiple tariff type definitions is well implemented.The test properly sets up two different tariff type definitions with different behaviors, adds them to the registry, and verifies that the behaviors can be retrieved correctly based on their respective types and behavior classes.
165-169: Error case for behavior retrieval is properly tested.The test correctly verifies that a
BehaviorNotFoundExceptionis thrown when attempting to retrieve a behavior that doesn't exist.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/product/price/PriceTypeDefinition.php (2)
106-110: 🛠️ Refactor suggestionMissing validation in end() method.
Despite the comment suggesting validation should happen, the
end()method doesn't perform any validation of the price type configuration.public function end(): PriceTypeDefinitionCollectionInterface { // Validate the PriceType and lock its state + if (!isset($this->unit)) { + throw new \LogicException('Unit must be defined before calling end()'); + } + if (!isset($this->description)) { + throw new \LogicException('Description must be defined before calling end()'); + } return $this->parent; }
120-123:⚠️ Potential issueFix undefined class reference in measuredWith method.
Static analysis indicates that the class
\hiqdev\billing\registry\measure\RcpTrafCollectordoesn't exist, which will cause runtime errors if this method is called.-public function measuredWith(\hiqdev\billing\registry\measure\RcpTrafCollector $param): self +public function measuredWith(/* Replace with correct interface or class */ $param): self { return $this; }🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 120-120: UndefinedClass
src/product/price/PriceTypeDefinition.php:120:34: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\measure\RcpTrafCollector does not exist (see https://psalm.dev/019)
🧹 Nitpick comments (4)
src/product/price/PriceTypeDefinition.php (4)
52-55: Consider documenting the purpose of the init hook.The
init()method is marked as a hook but doesn't provide context on when or how it should be used by extending classes.protected function init(): void { - // Hook + // Hook method for subclasses to initialize additional properties or perform setup operations }
57-69: Consider validating parameters in setters.The
unit()anddescription()methods don't validate their inputs. Fordescription(), consider checking for empty strings.public function description(string $description): self { + if (trim($description) === '') { + throw new \InvalidArgumentException('Description cannot be empty'); + } $this->description = $description; return $this; }
93-101: Consider adding validation for missing formatter definition.The
createQuantityFormatter()method doesn't check if the formatter definition has been set before using it.public function createQuantityFormatter( FractionQuantityData $data, ): QuantityFormatterInterface { + if (!isset($this->quantityFormatterDefinition)) { + throw new \LogicException('Quantity formatter definition has not been set. Call quantityFormatter() method first.'); + } return QuantityFormatterFactory::create( $this->getUnit()->createExternalUnit(), $this->quantityFormatterDefinition, $data, ); }
156-168: Translate non-English comments.The method has documentation in what appears to be Ukrainian. For international collaboration, consider translating these comments to English.
/** - * це параметер визначає агрегатну функцію яка застосовується для щоденно записаних ресурсів щоб визнизначти - * місячне споживання за яке потрібно пробілити клієнта + * This parameter defines the aggregate function that is applied to daily recorded resources + * to determine the monthly consumption for which the client needs to be billed * * @param AggregateInterface $aggregate * @return self */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/product/TariffTypeDefinitionInterface.php(1 hunks)src/product/behavior/HasBehaviorsInterface.php(1 hunks)src/product/price/PriceTypeDefinition.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/TariffTypeDefinitionInterface.php
🧰 Additional context used
🧬 Code Definitions (2)
src/product/behavior/HasBehaviorsInterface.php (1)
src/product/price/PriceTypeDefinition.php (2)
withBehaviors(140-143)hasBehavior(145-154)
src/product/price/PriceTypeDefinition.php (9)
src/product/Exception/AggregateNotDefinedException.php (1)
AggregateNotDefinedException(7-10)src/product/behavior/BehaviorPriceTypeDefinitionCollection.php (3)
BehaviorPriceTypeDefinitionCollection(8-19)__construct(10-13)end(15-18)src/product/invoice/InvoiceRepresentationCollection.php (3)
InvoiceRepresentationCollection(10-47)__construct(14-16)end(38-41)src/product/quantity/InvalidQuantityFormatterException.php (1)
InvalidQuantityFormatterException(7-10)src/product/TariffTypeDefinition.php (5)
__construct(22-26)tariffType(28-31)end(95-108)withBehaviors(77-82)hasBehavior(84-93)src/product/price/PriceTypeDefinitionCollection.php (2)
__construct(19-26)end(48-51)src/product/TariffTypeDefinitionInterface.php (2)
tariffType(14-14)end(28-28)src/product/price/PriceTypeDefinitionFactoryInterface.php (1)
create(10-14)src/product/behavior/HasBehaviorsInterface.php (2)
withBehaviors(7-7)hasBehavior(9-9)
🪛 GitHub Check: PHP 8.3
src/product/price/PriceTypeDefinition.php
[failure] 120-120: UndefinedClass
src/product/price/PriceTypeDefinition.php:120:34: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\measure\RcpTrafCollector does not exist (see https://psalm.dev/019)
🔇 Additional comments (6)
src/product/behavior/HasBehaviorsInterface.php (1)
5-10: Well-designed interface for behavior management.This interface provides a clean and consistent way to manage behaviors across different components in the billing system. The two methods are appropriately defined with clear return types and meaningful names.
src/product/price/PriceTypeDefinition.php (5)
24-36: Clean class implementation of HasBehaviorsInterface with well-defined properties.The class has a clear structure with properly typed properties for unit, description, formatters, and collections.
38-50: Constructor initializes collections properly.The constructor sets up the invoice and behavior collections, which follows good design practices by ensuring required dependencies are available from object creation.
82-91: Good implementation of class existence check.The implementation of class existence check for formatter classes is well done and addresses the previous review comment.
140-154: Good implementation of behavior methods.The
withBehaviors()andhasBehavior()methods correctly implement theHasBehaviorsInterfacerequirements and provide efficient behavior management functionality.
174-181: Well-implemented aggregate null check.The
getAggregate()method properly checks if the aggregate is null and throws an appropriate exception with a helpful message. This is a good implementation addressing a previous review comment.
| public function getDescription(): string | ||
| { | ||
| return $this->description; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing null check in getDescription().
The getDescription() method doesn't check if the description property has been initialized, which could lead to undefined property access.
public function getDescription(): string
{
+ if (!isset($this->description)) {
+ throw new \LogicException('Description has not been defined. Call description() method first.');
+ }
return $this->description;
}📝 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 getDescription(): string | |
| { | |
| return $this->description; | |
| } | |
| public function getDescription(): string | |
| { | |
| if (!isset($this->description)) { | |
| throw new \LogicException('Description has not been defined. Call description() method first.'); | |
| } | |
| return $this->description; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/product/price/PriceTypeDefinition.php (1)
63-68: Consider handling uninitialized$description.
IfgetDescription()is called before setting$description, it could cause issues (e.g., depending on strict typing level). Optionally, initialize it to an empty string or throw a logic exception when unset.src/product/price/PriceTypeDefinitionCollection.php (1)
44-51: Typo in psalm annotation.
At line 46,@plsam-return Mis likely a misspelling of@psalm-return M. Correcting it helps tools correctly parse annotations.src/product/price/PriceTypeDefinitionInterface.php (1)
39-44: Non-English doc comment.
Lines 39-41 are partly in a different language. For consistency, consider using a single language or providing an English translation.src/product/behavior/BehaviorCollectionInterface.php (1)
10-10: Consider documenting Traversable return type using generics for clarity.Although the docblock indicates this returns an array of
BehaviorInterface, you might enhance type safety with tools like Psalm or PHPStan by specifying something likeTraversable<BehaviorInterface>in the docblock.src/product/behavior/BehaviorCollection.php (2)
12-14: Document constructor parameter for maintainability.Currently, there’s no docblock explaining the constructor’s
$tariffTypeparameter. Adding a short docblock would help clarify why it’s declaredreadonlyand confirm it sets the tariff type on all attached behaviors.
21-28: Consider handling duplicate attachments.When you attach a
BehaviorInterfacemultiple times, it may lead to duplicates in the collection. This might be desired, but if not, consider using a set-like approach to prevent unwanted duplicates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/product/behavior/BehaviorCollection.php(1 hunks)src/product/behavior/BehaviorCollectionInterface.php(1 hunks)src/product/behavior/BehaviorPriceTypeDefinitionCollection.php(1 hunks)src/product/behavior/BehaviorTariffTypeCollection.php(1 hunks)src/product/price/PriceTypeDefinition.php(1 hunks)src/product/price/PriceTypeDefinitionCollection.php(1 hunks)src/product/price/PriceTypeDefinitionCollectionInterface.php(1 hunks)src/product/price/PriceTypeDefinitionFactory.php(1 hunks)src/product/price/PriceTypeDefinitionFactoryInterface.php(1 hunks)src/product/price/PriceTypeDefinitionInterface.php(1 hunks)src/product/price/PriceTypeStorage.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/product/price/PriceTypeDefinitionFactory.php
- src/product/price/PriceTypeDefinitionFactoryInterface.php
- src/product/behavior/BehaviorPriceTypeDefinitionCollection.php
- src/product/behavior/BehaviorTariffTypeCollection.php
- src/product/price/PriceTypeDefinitionCollectionInterface.php
- src/product/price/PriceTypeStorage.php
🧰 Additional context used
🧬 Code Definitions (3)
src/product/behavior/BehaviorCollection.php (4)
src/product/TariffTypeDefinition.php (2)
__construct(22-26)tariffType(28-31)src/product/behavior/BehaviorTariffTypeCollection.php (1)
__construct(10-13)src/product/behavior/BehaviorCollectionInterface.php (2)
getIterator(10-10)attach(12-12)src/product/behavior/BehaviorInterface.php (1)
setTariffType(12-12)
src/product/price/PriceTypeDefinition.php (7)
src/product/Exception/AggregateNotDefinedException.php (1)
AggregateNotDefinedException(7-10)src/product/behavior/BehaviorPriceTypeDefinitionCollection.php (3)
BehaviorPriceTypeDefinitionCollection(8-19)__construct(10-13)end(15-18)src/product/invoice/InvoiceRepresentationCollection.php (3)
InvoiceRepresentationCollection(10-47)__construct(14-16)end(38-41)src/product/quantity/InvalidQuantityFormatterException.php (1)
InvalidQuantityFormatterException(7-10)src/product/price/PriceTypeDefinitionCollection.php (2)
__construct(19-26)end(48-51)src/product/TariffTypeDefinitionInterface.php (2)
tariffType(14-14)end(28-28)src/product/price/PriceTypeDefinitionFactoryInterface.php (1)
create(10-14)
src/product/price/PriceTypeDefinitionCollection.php (4)
src/product/price/PriceTypeStorage.php (4)
PriceTypeStorage(7-39)getAll(23-33)add(13-18)count(35-38)src/product/price/PriceTypeDefinitionFactory.php (1)
create(10-16)src/product/price/PriceTypeDefinitionFactoryInterface.php (1)
create(10-14)src/product/TariffTypeDefinitionInterface.php (1)
tariffType(14-14)
🔇 Additional comments (7)
src/product/price/PriceTypeDefinition.php (2)
104-108: Implement or remove the validation note.
Currently, there’s a comment indicating that PriceType should be validated and locked, but no logic is present. Consider implementing the validation or removing the comment to avoid confusion.
118-121: Possible undefined class reference (repeated feedback).
This was flagged previously, but the\hiqdev\billing\registry\measure\RcpTrafCollectorclass may not exist or may be incorrectly referenced. Confirm its availability, or remove the method if it’s truly unneeded.src/product/price/PriceTypeDefinitionInterface.php (1)
29-30: Confirm usage or remove method.
measuredWith(\hiqdev\billing\registry\measure\RcpTrafCollector $param)may rely on a non-existent class. Moreover, there’s a “TODO” indicating uncertainty about needing it. Consider removing it or verifying the fully qualified class name is valid.src/product/behavior/BehaviorCollectionInterface.php (2)
5-6: Interface naming choice looks good.Declaring the
BehaviorCollectionInterfaceto extend\IteratorAggregateis a clean way to ensure iteration over a collection of behaviors. The naming is intuitive and aligns with established interface naming patterns.
14-14: Clarify the purpose ofend()method.The
end()method is declared without a docblock or return type. Please consider articulating its intended use, specifying a return type if necessary, or removing it if it’s not actually needed.src/product/behavior/BehaviorCollection.php (2)
7-9: Abstract class aligns with interface expectations.Declaring
BehaviorCollectionas abstract makes sense if you plan to extend it with custom logic while enforcing the contract fromBehaviorCollectionInterface. Good structure overall.
27-29: No implementation forend()method.The interface declares
end(), but this class does not implement it. If the intention is to let subclasses define it, ensure you document that in the interface or in this abstract class. Otherwise, consider providing a default or removing it if it’s never used.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/product/TariffTypeDefinition.php (1)
13-13:⚠️ Potential issueAdd missing template parameter to satisfy Psalm requirement
Psalm flags the class for missing a template parameter. Update the docblock to match the interface’s expected template param.Below is an example fix:
+/** + * @template T + * @implements TariffTypeDefinitionInterface<T> + */ class TariffTypeDefinition implements TariffTypeDefinitionInterface {🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 13-13: MissingTemplateParam
src/product/TariffTypeDefinition.php:13:39: MissingTemplateParam: hiqdev\php\billing\product\TariffTypeDefinition has missing template params when extending hiqdev\php\billing\product\TariffTypeDefinitionInterface, expecting 1 (see https://psalm.dev/182)🪛 GitHub Actions: Psalm Static Analysis
[error] 13-13: MissingTemplateParam: hiqdev\php\billing\product\TariffTypeDefinition has missing template params when extending hiqdev\php\billing\product\TariffTypeDefinitionInterface, expecting 1 (see https://psalm.dev/182)
🧹 Nitpick comments (4)
tests/unit/product/behavior/TariffTypeBehaviorRegistryTest.php (1)
11-14: Consider adding concise docblocks for class and methods
Docblocks can help clarify test objectives and improve readability for other developers.src/product/behavior/TariffTypeBehaviorRegistry.php (2)
10-15: Optional docblock for constructor
A short docblock describing the constructor parameters can help clarify how the registry is intended to manage behaviors.
22-31: Consider storing behaviors by class name for faster lookups
Currently, the entire collection is traversed to check for a matching behavior. If behaviors grow in number, a map keyed by class name could offer O(1) lookup.src/product/TariffTypeDefinition.php (1)
63-69: Implement logic for$suggesterClassor remove the placeholder
ThesetPricesSuggestermethod accepts a class string but does not validate or store it. Either implement suggester handling or remove the method to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/product/TariffTypeDefinition.php(1 hunks)src/product/behavior/TariffTypeBehaviorRegistry.php(1 hunks)tests/unit/product/behavior/TariffTypeBehaviorRegistryTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/product/TariffTypeDefinition.php (1)
Learnt from: SilverFire
PR: hiqdev/php-billing#93
File: src/product/BillingRegistry.php:5-5
Timestamp: 2025-03-27T16:47:33.914Z
Learning: The `hiqdev/php-billing` repository should not depend on `advancedhosters/billing-registry`. Use internal interfaces and implementations instead.
🧬 Code Definitions (1)
src/product/TariffTypeDefinition.php (4)
src/product/behavior/BehaviorTariffTypeCollection.php (3)
BehaviorTariffTypeCollection(8-19)__construct(10-13)end(15-18)src/product/Exception/ProductNotDefinedException.php (1)
ProductNotDefinedException(7-10)src/product/Exception/TariffTypeLockedException.php (1)
TariffTypeLockedException(5-8)src/product/price/PriceTypeDefinitionCollection.php (4)
PriceTypeDefinitionCollection(13-57)__construct(19-26)end(48-51)count(53-56)
🪛 GitHub Check: PHP 8.3
src/product/TariffTypeDefinition.php
[failure] 13-13: MissingTemplateParam
src/product/TariffTypeDefinition.php:13:39: MissingTemplateParam: hiqdev\php\billing\product\TariffTypeDefinition has missing template params when extending hiqdev\php\billing\product\TariffTypeDefinitionInterface, expecting 1 (see https://psalm.dev/182)
🪛 GitHub Actions: Psalm Static Analysis
src/product/TariffTypeDefinition.php
[error] 13-13: MissingTemplateParam: hiqdev\php\billing\product\TariffTypeDefinition has missing template params when extending hiqdev\php\billing\product\TariffTypeDefinitionInterface, expecting 1 (see https://psalm.dev/182)
🔇 Additional comments (2)
tests/unit/product/behavior/TariffTypeBehaviorRegistryTest.php (1)
29-29: Verify the presence ofTestBehavior
TestBehavior::classis referenced but not imported or defined within the provided code. Confirm it is present in your test suite and properly loaded.Also applies to: 34-34
src/product/TariffTypeDefinition.php (1)
78-103: Looks good overall
ThewithBehaviors(),hasBehavior(), andend()methods effectively manage behaviors and lock the definition as intended.
Summary by CodeRabbit
New Features
Tests
Chores