HP-2496: Extend TariffTypeDefinition with findBehaviorByClass method#104
Conversation
WalkthroughIntroduces an abstract BehaviorRegistry and concrete PriceTypeBehaviorRegistry, refactors TariffTypeBehaviorRegistry to extend BehaviorRegistry and renames getBehaviors()→withBehaviors(), removes TariffTypeDefinition::findPricesByTypeName, adjusts findBehaviorByClass typing, updates usages and tests, and cleans imports/exceptions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant TariffTypeDefinition
participant TariffTypeBehaviorRegistry
participant BehaviorCollection
Client->>TariffTypeDefinition: withBehaviors()
TariffTypeDefinition->>TariffTypeBehaviorRegistry: withBehaviors()
TariffTypeBehaviorRegistry->>BehaviorCollection: return collection
TariffTypeBehaviorRegistry-->>TariffTypeDefinition: BehaviorCollection
TariffTypeDefinition-->>Client: BehaviorCollection
sequenceDiagram
participant Client
participant PriceTypeDefinition
participant PriceTypeBehaviorRegistry
participant BehaviorRegistry
Client->>PriceTypeDefinition: hasBehavior(className) / findBehaviorByClass(class)
PriceTypeDefinition->>PriceTypeBehaviorRegistry: hasBehavior / findBehaviorByClass
PriceTypeBehaviorRegistry->>BehaviorRegistry: withBehaviors() / getBehaviorCollection()
BehaviorRegistry-->>PriceTypeBehaviorRegistry: result / behavior|null
PriceTypeBehaviorRegistry-->>PriceTypeDefinition: result / behavior|null
PriceTypeDefinition-->>Client: result / behavior|null
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
tests/unit/product/behavior/TariffTypeBehaviorRegistryTest.php (1)
35-36: Augment test coverage for findBehaviorByClassAfter attaching the behavior, also assert the registry can retrieve it by class. This exercises the new delegation path.
You can extend the test like this:
$behavior = $this->createMock(TestBehavior::class); $behaviorCollection = $this->manager->withBehaviors(); $behaviorCollection->attach($behavior); $this->assertTrue($this->manager->hasBehavior(TestBehavior::class)); + $this->assertSame($behavior, $this->manager->findBehaviorByClass(TestBehavior::class));src/product/behavior/HasBehaviorsInterface.php (1)
21-21: Specify intent and improve static analysis on findBehaviorByClassRemoving the return type is acceptable, but we should document the expected types and keep parameter naming consistent with hasBehavior.
Apply:
- public function findBehaviorByClass(string $class); + /** + * @psalm-param class-string<BehaviorInterface> $behaviorClassName + * @psalm-return BehaviorInterface|null + */ + public function findBehaviorByClass(string $behaviorClassName);This preserves runtime flexibility while giving Psalm/IDE precise info.
src/product/TariffTypeDefinition.php (1)
105-108: Add docblock and align parameter name for findBehaviorByClassMirror HasBehaviorsInterface docs and naming for consistency and better tooling.
- public function findBehaviorByClass(string $class) + /** + * @psalm-param class-string<\hiqdev\php\billing\product\behavior\BehaviorInterface> $behaviorClassName + * @psalm-return \hiqdev\php\billing\product\behavior\BehaviorInterface|null + */ + public function findBehaviorByClass(string $behaviorClassName) { - return $this->tariffTypeBehaviorRegistry->findBehaviorByClass($class); + return $this->tariffTypeBehaviorRegistry->findBehaviorByClass($behaviorClassName); }src/product/behavior/PriceTypeBehaviourRegistry.php (2)
21-26: Parameter naming clarity: this is a PriceTypeDefinition, not TariffTypeDefinitionThe parameter is misnamed; fix the name and psalm annotation to avoid confusion.
- /** - * @psalm-param T $tariffTypeDefinition - */ - public function __construct(PriceTypeDefinitionInterface $tariffTypeDefinition, TariffTypeInterface $tariffType) + /** + * @psalm-param T $priceTypeDefinition + */ + public function __construct(PriceTypeDefinitionInterface $priceTypeDefinition, TariffTypeInterface $tariffType) { - $this->behaviorCollection = new BehaviorPriceTypeDefinitionCollection($tariffTypeDefinition, $tariffType); + $this->behaviorCollection = new BehaviorPriceTypeDefinitionCollection($priceTypeDefinition, $tariffType); }
18-18: Consider marking the collection property readonlyThe collection reference itself is immutable after construction. Declaring it readonly communicates intent and prevents accidental reassignment.
- private BehaviorPriceTypeDefinitionCollection $behaviorCollection; + private readonly BehaviorPriceTypeDefinitionCollection $behaviorCollection;src/product/behavior/BehaviorRegistry.php (2)
13-22: Document expected input types for better static guaranteesClarify that we expect a behavior class-string and that the return is a BehaviorInterface|null.
- public function hasBehavior(string $behaviorClassName): bool + /** + * @psalm-param class-string<BehaviorInterface> $behaviorClassName + */ + public function hasBehavior(string $behaviorClassName): bool
24-33: Use a single accessor consistently and document typesUse getBehaviorCollection() here as well for consistency with hasBehavior() and lock(). Add psalm annotations to aid static analysis.
- public function findBehaviorByClass(string $class) + /** + * @psalm-param class-string<BehaviorInterface> $class + * @psalm-return BehaviorInterface|null + */ + public function findBehaviorByClass(string $class) { - foreach ($this->withBehaviors() as $behavior) { + foreach ($this->getBehaviorCollection() as $behavior) { if ($behavior instanceof $class) { return $behavior; } } return null; }src/product/behavior/TariffTypeBehaviorRegistry.php (1)
17-21: Docblock references stale method name (getBehaviors -> withBehaviors)Update the documentation to reflect the new API and that hasBehavior()/findBehaviorByClass() are inherited from BehaviorRegistry.
- * - Provides access to the behavior collection via getBehaviors(). - * - Allows checking for the existence of a specific behavior using hasBehavior(). + * - Provides access to the behavior collection via withBehaviors(). + * - Allows behavior lookup via hasBehavior()/findBehaviorByClass() (inherited from BehaviorRegistry).src/product/price/PriceTypeDefinition.php (2)
195-195: Typo in annotation: use @inheritdocFix minor typo to ensure tooling recognizes inheritance documentation.
- * @inerhitDoc + * @inheritdoc
184-192: Strengthen type hints via PHPDoc (class-string) for behavior lookupsDocument parameters as class-string to aid static analysis and improve API clarity.
- public function hasBehavior(string $behaviorClassName): bool + /** + * @param class-string $behaviorClassName + */ + public function hasBehavior(string $behaviorClassName): bool { return $this->behaviorRegistry->hasBehavior($behaviorClassName); } - public function findBehaviorByClass(string $class) + /** + * @param class-string $class + * @return mixed|null + */ + public function findBehaviorByClass(string $class) { return $this->behaviorRegistry->findBehaviorByClass($class); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/product/TariffTypeDefinition.php(3 hunks)src/product/TariffTypeDefinitionInterface.php(0 hunks)src/product/behavior/BehaviorRegistry.php(1 hunks)src/product/behavior/HasBehaviorsInterface.php(1 hunks)src/product/behavior/PriceTypeBehaviourRegistry.php(1 hunks)src/product/behavior/TariffTypeBehaviorRegistry.php(2 hunks)src/product/price/PriceTypeDefinition.php(7 hunks)tests/unit/product/behavior/TariffTypeBehaviorRegistryTest.php(2 hunks)
💤 Files with no reviewable changes (1)
- src/product/TariffTypeDefinitionInterface.php
🧰 Additional context used
🧬 Code Graph Analysis (6)
tests/unit/product/behavior/TariffTypeBehaviorRegistryTest.php (3)
src/product/behavior/BehaviorTariffTypeCollection.php (1)
BehaviorTariffTypeCollection(13-36)src/product/behavior/HasBehaviorsInterface.php (1)
withBehaviors(17-17)src/product/behavior/TariffTypeBehaviorRegistry.php (1)
withBehaviors(47-50)
src/product/behavior/HasBehaviorsInterface.php (3)
src/product/behavior/BehaviorRegistry.php (1)
findBehaviorByClass(24-33)src/product/TariffTypeDefinition.php (1)
findBehaviorByClass(105-108)src/product/price/PriceTypeDefinition.php (1)
findBehaviorByClass(189-192)
src/product/TariffTypeDefinition.php (4)
src/Exception/LogicException.php (1)
LogicException(5-8)src/product/behavior/HasBehaviorsInterface.php (3)
withBehaviors(17-17)hasBehavior(19-19)findBehaviorByClass(21-21)src/product/behavior/TariffTypeBehaviorRegistry.php (1)
withBehaviors(47-50)src/product/behavior/BehaviorRegistry.php (2)
hasBehavior(13-22)findBehaviorByClass(24-33)
src/product/behavior/BehaviorRegistry.php (5)
src/product/behavior/PriceTypeBehaviourRegistry.php (2)
getBehaviorCollection(36-39)withBehaviors(31-34)src/product/behavior/TariffTypeBehaviorRegistry.php (2)
getBehaviorCollection(52-55)withBehaviors(47-50)src/product/behavior/HasBehaviorsInterface.php (3)
hasBehavior(19-19)findBehaviorByClass(21-21)withBehaviors(17-17)src/product/TariffTypeDefinition.php (3)
hasBehavior(100-103)findBehaviorByClass(105-108)withBehaviors(93-98)src/product/price/PriceTypeDefinition.php (3)
hasBehavior(184-187)findBehaviorByClass(189-192)withBehaviors(177-182)
src/product/behavior/PriceTypeBehaviourRegistry.php (7)
src/product/behavior/BehaviorRegistry.php (2)
BehaviorRegistry(9-39)getBehaviorCollection(11-11)src/product/behavior/BehaviorPriceTypeDefinitionCollection.php (1)
BehaviorPriceTypeDefinitionCollection(14-40)src/product/TariffTypeDefinition.php (3)
__construct(29-33)tariffType(35-38)withBehaviors(93-98)src/product/behavior/TariffTypeBehaviorRegistry.php (3)
__construct(39-42)withBehaviors(47-50)getBehaviorCollection(52-55)src/product/price/PriceTypeDefinition.php (2)
__construct(59-69)withBehaviors(177-182)src/product/TariffTypeDefinitionInterface.php (1)
tariffType(18-18)src/product/behavior/HasBehaviorsInterface.php (1)
withBehaviors(17-17)
src/product/behavior/TariffTypeBehaviorRegistry.php (6)
src/product/behavior/BehaviorRegistry.php (2)
BehaviorRegistry(9-39)getBehaviorCollection(11-11)src/product/behavior/HasBehaviorsInterface.php (1)
withBehaviors(17-17)src/product/TariffTypeDefinition.php (1)
withBehaviors(93-98)src/product/behavior/PriceTypeBehaviourRegistry.php (2)
withBehaviors(31-34)getBehaviorCollection(36-39)src/product/price/PriceTypeDefinition.php (1)
withBehaviors(177-182)src/product/behavior/BehaviorTariffTypeCollection.php (1)
BehaviorTariffTypeCollection(13-36)
🔇 Additional comments (7)
tests/unit/product/behavior/TariffTypeBehaviorRegistryTest.php (1)
24-24: Rename usage looks correctSwitch to withBehaviors() aligns with the new API. No issues here.
src/product/TariffTypeDefinition.php (2)
97-98: Correct delegation to registrySwitching to $this->tariffTypeBehaviorRegistry->withBehaviors() is consistent with the new registry abstraction.
12-12: Confirm which LogicException to use (global vs project-specific)You currently import use LogicException; which maps to \LogicException. The project also defines hiqdev\php\billing\Exception\LogicException. If the intent is to throw a domain-specific exception (easier to catch library errors), import that instead.
If desired, switch to the project exception:
- use LogicException; + use hiqdev\php\billing\Exception\LogicException;Then Line 116 remains throw new LogicException(...);
Also applies to: 116-116
src/product/behavior/TariffTypeBehaviorRegistry.php (3)
29-29: Good move: centralize behavior logic via BehaviorRegistryExtending BehaviorRegistry removes duplication and aligns with the new registry pattern.
47-50: withBehaviors() impl is correct and type-safeReturning the concrete BehaviorTariffTypeCollection is appropriate and compatible with HasBehaviorsInterface expectations.
52-55: Correct fulfillment of abstract contractgetBehaviorCollection() cleanly exposes the underlying collection for the base class.
src/product/price/PriceTypeDefinition.php (1)
54-55: Confirm TariffTypeDefinition exposes findBehaviorByClass and delegation is wired correctlyAll delegation methods are in place and TariffTypeDefinition now directly exposes
findBehaviorByClass:
src/product/TariffTypeDefinition.php lines 105–107:
public function findBehaviorByClass(string $class) { return $this->tariffTypeBehaviorRegistry->findBehaviorByClass($class); }Delegation for
withBehaviors(),hasBehavior(),lock()and others remain correctly routed throughTariffTypeBehaviorRegistry.Note: the registry class names use inconsistent spelling—Price uses
PriceTypeBehaviourRegistry(UK) while Tariff usesTariffTypeBehaviorRegistry(US). If this wasn’t intentional, consider unifying the naming across both registries.
| /** | ||
| * @template T as PriceTypeDefinitionInterface | ||
| */ | ||
| final class PriceTypeBehaviourRegistry extends BehaviorRegistry |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Naming consistency: Behaviour ➜ Behavior
The codebase predominantly uses “Behavior”. Introduce PriceTypeBehaviorRegistry to match TariffTypeBehaviorRegistry and other classes. Otherwise, you’ll have mixed spellings across APIs and types.
-final class PriceTypeBehaviourRegistry extends BehaviorRegistry
+final class PriceTypeBehaviorRegistry extends BehaviorRegistryFollow-up: rename the file to PriceTypeBehaviorRegistry.php and update all references.
🏁 Script executed:
#!/bin/bash
# Find all British spelling occurrences to update along with the class/file rename
rg -n "PriceTypeBehaviourRegistry|BehaviourRegistry|Behaviour" --glob '!vendor'Length of output: 555
Rename PriceTypeBehaviourRegistry to PriceTypeBehaviorRegistry for naming consistency
To align with the rest of the codebase (which uses the American spelling “Behavior”), rename the class, file, and all references:
• src/product/behavior/PriceTypeBehaviourRegistry.php
-final class PriceTypeBehaviourRegistry extends BehaviorRegistry
+final class PriceTypeBehaviorRegistry extends BehaviorRegistry• Rename file → src/product/behavior/PriceTypeBehaviorRegistry.php
• src/product/price/PriceTypeDefinition.php
-use hiqdev\php\billing\product\behavior\PriceTypeBehaviourRegistry;
+use hiqdev\php\billing\product\behavior\PriceTypeBehaviorRegistry;
-private readonly PriceTypeBehaviourRegistry $behaviorRegistry;
+private readonly PriceTypeBehaviorRegistry $behaviorRegistry;
-$this->behaviorRegistry = new PriceTypeBehaviourRegistry($this, $tariffType);
+$this->behaviorRegistry = new PriceTypeBehaviorRegistry($this, $tariffType);After renaming, run a global search for “BehaviourRegistry” to catch any remaining imports or references.
🤖 Prompt for AI Agents
In src/product/behavior/PriceTypeBehaviourRegistry.php at line 13, rename the
class from PriceTypeBehaviourRegistry to PriceTypeBehaviorRegistry to maintain
naming consistency with the American spelling used elsewhere. Rename the file
accordingly to PriceTypeBehaviorRegistry.php and update all references and
imports in the codebase, including in src/product/price/PriceTypeDefinition.php.
After these changes, perform a global search for "BehaviourRegistry" to ensure
no remaining references exist.
| use hiqdev\php\billing\product\behavior\BehaviorCollectionInterface; | ||
| use hiqdev\php\billing\product\behavior\BehaviorInterface; | ||
| use hiqdev\php\billing\product\behavior\HasBehaviorsInterface; | ||
| use hiqdev\php\billing\product\behavior\PriceTypeBehaviourRegistry; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Naming consistency: Behaviour vs Behavior
The codebase mixes “Behaviour” and “Behavior”. If not intentional, consider standardizing to a single variant to reduce confusion and accidental mis-imports.
To assess the scope, run:
Also applies to: 54-55
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "Behaviour|Behavior" src | sed -E 's/:.*//' | sort -uLength of output: 953
Standardize “Behavior” vs “Behaviour” Spelling
The codebase currently mixes British and American spellings. Almost all behavior-related classes use Behavior (US), except:
src/product/behavior/PriceTypeBehaviourRegistry.php(class, file name, and references)- import in
src/product/price/PriceTypeDefinition.php(line 8)
To avoid confusion and accidental mis-imports, pick one variant—ideally Behavior to match the directory and existing classes—and update:
- Rename
PriceTypeBehaviourRegistry.php→PriceTypeBehaviorRegistry.php - Update its class name and namespace accordingly
- Adjust all imports and references (e.g., in
PriceTypeDefinition.php) - Ensure tests and any related docs use the chosen spelling
🤖 Prompt for AI Agents
In src/product/price/PriceTypeDefinition.php at line 8, the import uses the
British spelling "Behaviour" which is inconsistent with the rest of the codebase
that uses the American spelling "Behavior". Rename the file
src/product/behavior/PriceTypeBehaviourRegistry.php to
PriceTypeBehaviorRegistry.php, update the class name and namespace inside that
file to use "Behavior", and change all imports and references accordingly,
including the import in PriceTypeDefinition.php at line 8. Also, update any
tests and documentation to use the standardized "Behavior" spelling.
| use hiqdev\php\billing\product\TariffTypeDefinitionInterface; | ||
| use hiqdev\php\billing\product\trait\HasLock; | ||
| use hiqdev\php\billing\type\TypeInterface; | ||
| use function class_exists; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate formatter contract, not just existence
class_exists prevents fatals, but validating the class implements QuantityFormatterInterface will fail earlier and with a clearer message.
- if (!class_exists($formatterClass)) {
+ if (!class_exists($formatterClass)) {
throw new InvalidQuantityFormatterException("Formatter class $formatterClass does not exist");
}
+ if (!is_subclass_of($formatterClass, QuantityFormatterInterface::class)) {
+ throw new InvalidQuantityFormatterException(sprintf(
+ 'Formatter class %s must implement %s',
+ $formatterClass,
+ QuantityFormatterInterface::class
+ ));
+ }Also applies to: 107-112
🤖 Prompt for AI Agents
In src/product/price/PriceTypeDefinition.php at line 24 and also lines 107 to
112, the current code uses class_exists to check for the formatter class
existence, which prevents fatal errors but does not ensure the class implements
QuantityFormatterInterface. Update the validation to check that the class both
exists and implements QuantityFormatterInterface, throwing an early and clear
exception if it does not. This will improve error clarity and contract
enforcement.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/product/behavior/BehaviorRegistry.php (1)
20-29: Minor: reduce duplication by delegating to findBehaviorByClass.Same traversal twice. Reuse findBehaviorByClass for clarity and maintainability.
- public function hasBehavior(string $behaviorClassName): bool - { - foreach ($this->getBehaviorCollection() as $behavior) { - if ($behavior instanceof $behaviorClassName) { - return true; - } - } - - return false; - } + /** + * @param class-string $behaviorClassName + */ + public function hasBehavior(string $behaviorClassName): bool + { + return $this->findBehaviorByClass($behaviorClassName) !== null; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/product/behavior/BehaviorRegistry.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/behavior/BehaviorRegistry.php
[failure] 16-16: InvalidTemplateParam
src/product/behavior/BehaviorRegistry.php:16:16: InvalidTemplateParam: Extended template param TParentContext of hiqdev\php\billing\product\behavior\BehaviorCollectionInterface<TParentCollection:hiqdev\php\billing\product\behavior\BehaviorRegistry as mixed> expects type hiqdev\php\billing\product\TariffTypeDefinitionInterface|hiqdev\php\billing\product\price\PriceTypeDefinitionInterface, type TParentCollection:hiqdev\php\billing\product\behavior\BehaviorRegistry as mixed given (see https://psalm.dev/183)
[failure] 13-13: InvalidTemplateParam
src/product/behavior/BehaviorRegistry.php:13:62: InvalidTemplateParam: Extended template param TParentCollection expects type hiqdev\php\billing\product\TariffTypeDefinitionInterface|hiqdev\php\billing\product\price\PriceTypeDefinitionInterface, type TParentCollection:hiqdev\php\billing\product\behavior\BehaviorRegistry as mixed given (see https://psalm.dev/183)
[failure] 13-13: InvalidTemplateParam
src/product/behavior/BehaviorRegistry.php:13:62: InvalidTemplateParam: Cannot extend a strictly-enforced parent template param TParentCollection with constraint hiqdev\php\billing\product\TariffTypeDefinitionInterface|hiqdev\php\billing\product\price\PriceTypeDefinitionInterface with a child template param TParentCollection with different constraint mixed (see https://psalm.dev/183)
🪛 GitHub Actions: Psalm Static Analysis
src/product/behavior/BehaviorRegistry.php
[error] 13-13: Psalm InvalidTemplateParam: Cannot extend a strictly-enforced parent template param TParentCollection with constraint hiqdev\php\billing\product\TariffTypeDefinitionInterface|hiqdev\php\billing\product\price\PriceTypeDefinitionInterface with a child template param TParentCollection with different constraint mixed.
🔇 Additional comments (1)
src/product/behavior/BehaviorRegistry.php (1)
7-7: Import path verifiedHasLockInterface is declared in src/product/trait/HasLockInterface.php under
namespace hiqdev\php\billing\product\trait; theusestatement is correct and will autoload as expected.
| public function findBehaviorByClass(string $class) | ||
| { | ||
| foreach ($this->getBehaviorCollection() as $behavior) { | ||
| if ($behavior instanceof $class) { | ||
| return $behavior; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen typing for findBehaviorByClass with class-string and generic return.
Improve static analysis by annotating class-string input and a templated return type.
- public function findBehaviorByClass(string $class)
+ /**
+ * @template TBehavior of object
+ * @param class-string<TBehavior> $class
+ * @return TBehavior|null
+ */
+ public function findBehaviorByClass(string $class)
{
foreach ($this->getBehaviorCollection() as $behavior) {
if ($behavior instanceof $class) {
return $behavior;
}
}
return null;
}🤖 Prompt for AI Agents
In src/product/behavior/BehaviorRegistry.php around lines 31 to 40, the method
findBehaviorByClass lacks precise typing for its input and return value. Update
the PHPDoc to specify the parameter as a class-string of a generic type T and
the return type as either an instance of T or null. This will enhance static
analysis and type safety by clearly indicating the expected input and output
types.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/product/behavior/PriceTypeBehaviorRegistry.php (1)
19-19: Optional: Make the collection property readonly (PHP ≥ 8.1)The field is initialized once in the constructor and never reassigned. Marking it
readonlyimproves immutability guarantees.- private BehaviorPriceTypeDefinitionCollection $behaviorCollection; + private readonly BehaviorPriceTypeDefinitionCollection $behaviorCollection;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/product/behavior/BehaviorRegistry.php(1 hunks)src/product/behavior/PriceTypeBehaviorRegistry.php(1 hunks)src/product/behavior/TariffTypeBehaviorRegistry.php(2 hunks)src/product/price/PriceTypeDefinition.php(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/product/behavior/TariffTypeBehaviorRegistry.php
- src/product/behavior/BehaviorRegistry.php
- src/product/price/PriceTypeDefinition.php
🔇 Additional comments (1)
src/product/behavior/PriceTypeBehaviorRegistry.php (1)
37-40: LGTM: correct protected override returning the interfaceClean override; returning
BehaviorCollectionInterfacekeeps the base contract intact while encapsulating the concrete collection.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/product/behavior/TariffTypeBehaviorRegistry.php (2)
53-56: Add precise generics to return type doc for getBehaviorCollection()To help Psalm/IDE inference, annotate the generic on the interface type.
+ /** + * @return BehaviorCollectionInterface<T> + */ protected function getBehaviorCollection(): BehaviorCollectionInterface { return $this->behaviorCollection; }
17-25: Docblock is stale: reference withBehaviors() and findBehaviorByClass()The docs still mention getBehaviors(). Also, consider noting findBehaviorByClass() availability via the base class.
- * - Provides access to the behavior collection via getBehaviors(). - * - Allows checking for the existence of a specific behavior using hasBehavior(). + * - Provides access to the behavior collection via withBehaviors(). + * - Exposes behavior lookup helpers (e.g., hasBehavior(), findBehaviorByClass()) via the parent BehaviorRegistry.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/product/behavior/HasBehaviorsInterface.php(1 hunks)src/product/behavior/PriceTypeBehaviorRegistry.php(1 hunks)src/product/behavior/TariffTypeBehaviorRegistry.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/product/behavior/PriceTypeBehaviorRegistry.php
- src/product/behavior/HasBehaviorsInterface.php
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/behavior/TariffTypeBehaviorRegistry.php
[failure] 30-30: InvalidTemplateParam
src/product/behavior/TariffTypeBehaviorRegistry.php:30:13: InvalidTemplateParam: Cannot extend an invariant template param TParentCollection into a covariant context (see https://psalm.dev/183)
🪛 GitHub Actions: Psalm Static Analysis
src/product/behavior/TariffTypeBehaviorRegistry.php
[error] 30-30: Psalm: InvalidTemplateParam: Cannot extend an invariant template param TParentCollection into a covariant context (see https://psalm.dev/183)
| public function withBehaviors(): BehaviorTariffTypeCollection | ||
| { | ||
| return $this->behaviorCollection; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
withBehaviors() rename: verify all call sites migrated from getBehaviors()
Rename looks good. Please ensure no usages of getBehaviors() remain.
Run this script to find leftovers and confirm new usages:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for old API getBehaviors("
rg -n --hidden --no-ignore -S '\bgetBehaviors\(' || true
echo
echo "Searching for new API withBehaviors("
rg -n --hidden --no-ignore -S '\bwithBehaviors\(' || trueLength of output: 2746
withBehaviors() rename verification
All call-sites of the old getBehaviors() API have been updated to withBehaviors(). The only remaining reference is in the docblock of the registry class:
• src/product/behavior/TariffTypeBehaviorRegistry.php:19
Change “Provides access to the behavior collection via getBehaviors().” to “via withBehaviors().”
🤖 Prompt for AI Agents
In src/product/behavior/TariffTypeBehaviorRegistry.php around lines 19, update
the class docblock to replace the phrase "Provides access to the behavior
collection via getBehaviors()." with "Provides access to the behavior collection
via withBehaviors()." to reflect the method rename accurately.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/product/behavior/BehaviorRegistry.php (1)
33-42: Strengthen typing for findBehaviorByClass (class-string + generic return).Annotate with method-level template and class-string param to improve static analysis.
- public function findBehaviorByClass(string $class) + /** + * @template TBehavior of object + * @param class-string<TBehavior> $class + * @return TBehavior|null + */ + public function findBehaviorByClass(string $class)
🧹 Nitpick comments (1)
src/product/behavior/BehaviorRegistry.php (1)
22-31: Tighten input typing for hasBehavior with class-string.Clarify that the argument is a class-string for better static analysis.
- public function hasBehavior(string $behaviorClassName): bool + /** + * @param class-string $behaviorClassName + */ + public function hasBehavior(string $behaviorClassName): bool
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/product/behavior/BehaviorRegistry.php(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-13T12:20:09.570Z
Learnt from: SilverFire
PR: hiqdev/php-billing#93
File: src/product/BillingRegistry.php:5-5
Timestamp: 2025-01-13T12:20:09.570Z
Learning: The `hiqdev/php-billing` repository should not depend on `advancedhosters/billing-registry`. Use internal interfaces and implementations instead.
Applied to files:
src/product/behavior/BehaviorRegistry.php
🧬 Code Graph Analysis (1)
src/product/behavior/BehaviorRegistry.php (4)
src/product/behavior/TariffTypeBehaviorRegistry.php (1)
getBehaviorCollection(53-56)src/product/behavior/HasBehaviorsInterface.php (2)
hasBehavior(19-19)findBehaviorByClass(26-26)src/product/TariffTypeDefinition.php (2)
hasBehavior(100-103)findBehaviorByClass(105-108)src/product/price/PriceTypeDefinition.php (2)
hasBehavior(184-187)findBehaviorByClass(189-192)
🪛 GitHub Check: PHP 8.3
src/product/behavior/BehaviorRegistry.php
[failure] 15-15: InvalidTemplateParam
src/product/behavior/BehaviorRegistry.php:15:62: InvalidTemplateParam: Cannot extend an invariant template param TParentCollection into a covariant context (see https://psalm.dev/183)
🪛 GitHub Actions: Psalm Static Analysis
src/product/behavior/BehaviorRegistry.php
[error] 15-15: Psalm: InvalidTemplateParam: Cannot extend an invariant template param TParentCollection into a covariant context (see https://psalm.dev/183)
🔇 Additional comments (1)
src/product/behavior/BehaviorRegistry.php (1)
44-47: Lock delegation LGTM.Delegates to underlying collection; matches HasLockInterface intent.
| /** | ||
| * @template-covariant TParentCollection of PriceTypeDefinitionInterface|TariffTypeDefinitionInterface | ||
| * @implements HasBehaviorsInterface<TParentCollection> | ||
| */ | ||
| abstract class BehaviorRegistry implements HasLockInterface, HasBehaviorsInterface | ||
| { |
There was a problem hiding this comment.
Fix template variance to satisfy Psalm (blocks CI).
TParentCollection is declared covariant, but HasBehaviorsInterface expects an invariant template. Make it invariant.
/**
- * @template-covariant TParentCollection of PriceTypeDefinitionInterface|TariffTypeDefinitionInterface
+ * @template TParentCollection of PriceTypeDefinitionInterface|TariffTypeDefinitionInterface
* @implements HasBehaviorsInterface<TParentCollection>
*/
abstract class BehaviorRegistry implements HasLockInterface, HasBehaviorsInterface📝 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.
| /** | |
| * @template-covariant TParentCollection of PriceTypeDefinitionInterface|TariffTypeDefinitionInterface | |
| * @implements HasBehaviorsInterface<TParentCollection> | |
| */ | |
| abstract class BehaviorRegistry implements HasLockInterface, HasBehaviorsInterface | |
| { | |
| /** | |
| - * @template-covariant TParentCollection of PriceTypeDefinitionInterface|TariffTypeDefinitionInterface | |
| + * @template TParentCollection of PriceTypeDefinitionInterface|TariffTypeDefinitionInterface | |
| * @implements HasBehaviorsInterface<TParentCollection> | |
| */ | |
| abstract class BehaviorRegistry implements HasLockInterface, HasBehaviorsInterface | |
| { | |
| // ... | |
| } |
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 15-15: InvalidTemplateParam
src/product/behavior/BehaviorRegistry.php:15:62: InvalidTemplateParam: Cannot extend an invariant template param TParentCollection into a covariant context (see https://psalm.dev/183)
🪛 GitHub Actions: Psalm Static Analysis
[error] 15-15: Psalm: InvalidTemplateParam: Cannot extend an invariant template param TParentCollection into a covariant context (see https://psalm.dev/183)
🤖 Prompt for AI Agents
In src/product/behavior/BehaviorRegistry.php around lines 11 to 16, the template
parameter TParentCollection is declared as covariant, but the
HasBehaviorsInterface requires it to be invariant. Change the template
annotation for TParentCollection from covariant to invariant to satisfy Psalm
and unblock CI.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/product/behavior/PriceTypeBehaviorRegistry.php (1)
10-13: Fix Psalm generics: declare template T and use it in @extendsT is referenced in property/param/return docblocks but never declared, causing the Psalm UndefinedDocblockClass errors. Also, per prior feedback, propagate the subclass template to the parent.
-/** - * @extends BehaviorRegistry<PriceTypeDefinitionInterface> - */ +/** + * @template T of PriceTypeDefinitionInterface + * @extends BehaviorRegistry<T> + */This will resolve the reported issues at Lines 18, 21, and 29 and properly wire generics across the hierarchy.
🧹 Nitpick comments (1)
src/product/behavior/PriceTypeBehaviorRegistry.php (1)
18-18: Make the collection property readonly (immutability by construction)The registry is immutable post-construction. Mark the property readonly to encode the invariant and aid static analysis.
- private BehaviorPriceTypeDefinitionCollection $behaviorCollection; + private readonly BehaviorPriceTypeDefinitionCollection $behaviorCollection;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/product/behavior/PriceTypeBehaviorRegistry.php(1 hunks)src/product/behavior/TariffTypeBehaviorRegistry.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/behavior/TariffTypeBehaviorRegistry.php
🧰 Additional context used
🪛 GitHub Check: PHP 8.3
src/product/behavior/PriceTypeBehaviorRegistry.php
[failure] 29-29: UndefinedDocblockClass
src/product/behavior/PriceTypeBehaviorRegistry.php:29:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\php\billing\product\behavior\T does not exist (see https://psalm.dev/200)
[failure] 21-21: UndefinedDocblockClass
src/product/behavior/PriceTypeBehaviorRegistry.php:21:21: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\php\billing\product\behavior\T does not exist (see https://psalm.dev/200)
[failure] 18-18: UndefinedDocblockClass
src/product/behavior/PriceTypeBehaviorRegistry.php:18:13: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\php\billing\product\behavior\T does not exist (see https://psalm.dev/200)
🔇 Additional comments (2)
src/product/behavior/PriceTypeBehaviorRegistry.php (2)
31-34: withBehaviors() is clear and correctly typedReturn type and variance are appropriate; exposing the specialized collection aligns with usage.
36-39: Correct override of abstract accessorReturns the expected BehaviorCollectionInterface; this enables base class features (hasBehavior/findBehaviorByClass/locking).
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/product/behavior/TariffTypeBehaviorRegistry.php (1)
17-21: Consistently apply API rename getBehaviors() → withBehaviors()The API rename hasn’t been applied everywhere—besides updating the docblock in TariffTypeBehaviorRegistry.php, you also need to rename the method and its interface.
Files to update:
- src/product/behavior/TariffTypeBehaviorRegistry.php (docblock)
- src/product/Application/BillingRegistryService.php (method signature)
- src/product/Application/BillingRegistryServiceInterface.php (interface signature)
Proposed diffs:
--- a/src/product/behavior/TariffTypeBehaviorRegistry.php +++ b/src/product/behavior/TariffTypeBehaviorRegistry.php @@ -17,7 +17,7 @@ * Purpose: * - Stores a collection of behaviors associated with a TariffType. - * - Provides access to the behavior collection via getBehaviors(). + * - Provides access to the behavior collection via withBehaviors(). * - Allows checking for the existence of a specific behavior using hasBehavior(). *--- a/src/product/Application/BillingRegistryService.php +++ b/src/product/Application/BillingRegistryService.php @@ -111,7 +111,7 @@ /** * Retrieve behaviors registered for a given wrapper. - * @return \Generator - */ - public function getBehaviors(string $behaviorClassWrapper): \Generator + * @return \Generator + */ + public function withBehaviors(string $behaviorClassWrapper): \Generator { // … }--- a/src/product/Application/BillingRegistryServiceInterface.php +++ b/src/product/Application/BillingRegistryServiceInterface.php @@ -58,7 +58,7 @@ * @param string $behaviorClassWrapper * @return Generator */ - public function getBehaviors(string $behaviorClassWrapper): Generator; + public function withBehaviors(string $behaviorClassWrapper): Generator;
♻️ Duplicate comments (3)
src/product/behavior/BehaviorRegistry.php (2)
22-31: Strengthen typing of findBehaviorByClass with class-string and generic return.Improves static analysis and callers’ DX; keeps signature compatible with the interface.
- public function findBehaviorByClass(string $class) + /** + * @template TBehavior of object + * @param class-string<TBehavior> $class + * @return TBehavior|null + */ + public function findBehaviorByClass(string $class) { foreach ($this->withBehaviors() as $behavior) { if ($behavior instanceof $class) { return $behavior; } } return null; }
5-9: Add constrained template + @implements and required imports to satisfy Psalm generics (blocks CI).BehaviorRegistry implements a generic HasBehaviorsInterface but declares no template. Psalm errors: MissingTemplateParam/InvalidTemplateParam. Constrain and wire the template via @implements and import the constrained types.
Apply:
namespace hiqdev\php\billing\product\behavior; use hiqdev\php\billing\product\trait\HasLockInterface; +use hiqdev\php\billing\product\TariffTypeDefinitionInterface; +use hiqdev\php\billing\product\price\PriceTypeDefinitionInterface; -abstract class BehaviorRegistry implements HasLockInterface, HasBehaviorsInterface +/** + * @template TParentCollection of TariffTypeDefinitionInterface|PriceTypeDefinitionInterface + * @implements HasBehaviorsInterface<TParentCollection> + */ +abstract class BehaviorRegistry implements HasLockInterface, HasBehaviorsInterfacesrc/product/behavior/TariffTypeBehaviorRegistry.php (1)
27-31: Fix template variance and map child template to parent with @extends.Parent BehaviorRegistry’s template is invariant; child declares covariant T — Psalm flags InvalidTemplateParam. Make T invariant and explicitly extend the parent with T.
- * @template-covariant T of TariffTypeDefinitionInterface + * @template T of TariffTypeDefinitionInterface + * @extends BehaviorRegistry<T>
🧹 Nitpick comments (2)
src/product/behavior/BehaviorRegistry.php (1)
11-20: Annotate hasBehavior arg as class-string for better static analysis.Small DX win; no runtime changes.
- public function hasBehavior(string $behaviorClassName): bool + /** + * @param class-string $behaviorClassName + */ + public function hasBehavior(string $behaviorClassName): bool { foreach ($this->withBehaviors() as $behavior) { if ($behavior instanceof $behaviorClassName) { return true; } } return false; }src/product/behavior/TariffTypeBehaviorRegistry.php (1)
52-55: Add templated return annotation for getBehaviorCollection().Helps Psalm keep T flowing through the registry.
- protected function getBehaviorCollection(): BehaviorCollectionInterface + /** + * @return BehaviorCollectionInterface<T> + */ + protected function getBehaviorCollection(): BehaviorCollectionInterface { return $this->behaviorCollection; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/product/behavior/BehaviorRegistry.php(1 hunks)src/product/behavior/PriceTypeBehaviorRegistry.php(1 hunks)src/product/behavior/TariffTypeBehaviorRegistry.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/product/behavior/PriceTypeBehaviorRegistry.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-13T12:20:09.570Z
Learnt from: SilverFire
PR: hiqdev/php-billing#93
File: src/product/BillingRegistry.php:5-5
Timestamp: 2025-01-13T12:20:09.570Z
Learning: The `hiqdev/php-billing` repository should not depend on `advancedhosters/billing-registry`. Use internal interfaces and implementations instead.
Applied to files:
src/product/behavior/BehaviorRegistry.php
🧬 Code Graph Analysis (2)
src/product/behavior/BehaviorRegistry.php (4)
src/product/behavior/HasBehaviorsInterface.php (3)
hasBehavior(19-19)withBehaviors(17-17)findBehaviorByClass(26-26)src/product/price/PriceTypeDefinition.php (3)
hasBehavior(184-187)withBehaviors(177-182)findBehaviorByClass(189-192)src/product/behavior/PriceTypeBehaviorRegistry.php (1)
withBehaviors(31-34)src/product/behavior/TariffTypeBehaviorRegistry.php (1)
withBehaviors(47-50)
src/product/behavior/TariffTypeBehaviorRegistry.php (6)
src/product/behavior/BehaviorRegistry.php (1)
BehaviorRegistry(9-37)src/product/behavior/PriceTypeBehaviorRegistry.php (2)
withBehaviors(31-34)getBehaviorCollection(36-39)src/product/behavior/HasBehaviorsInterface.php (1)
withBehaviors(17-17)src/product/TariffTypeDefinition.php (1)
withBehaviors(93-98)src/product/price/PriceTypeDefinition.php (1)
withBehaviors(177-182)src/product/behavior/BehaviorTariffTypeCollection.php (1)
BehaviorTariffTypeCollection(13-36)
🪛 GitHub Check: PHP 8.3
src/product/behavior/BehaviorRegistry.php
[failure] 9-9: InvalidTemplateParam
src/product/behavior/BehaviorRegistry.php:9:62: InvalidTemplateParam: Cannot extend a strictly-enforced parent template param TParentCollection with a non-template type (see https://psalm.dev/183)
[failure] 9-9: MissingTemplateParam
src/product/behavior/BehaviorRegistry.php:9:62: MissingTemplateParam: hiqdev\php\billing\product\behavior\BehaviorRegistry requires the same number of template params as hiqdev\php\billing\product\behavior\HasBehaviorsInterface but saw 0 (see https://psalm.dev/182)
[failure] 9-9: MissingTemplateParam
src/product/behavior/BehaviorRegistry.php:9:62: MissingTemplateParam: hiqdev\php\billing\product\behavior\BehaviorRegistry has missing template params when extending hiqdev\php\billing\product\behavior\HasBehaviorsInterface, expecting 1 (see https://psalm.dev/182)
🪛 GitHub Actions: Psalm Static Analysis
src/product/behavior/BehaviorRegistry.php
[error] 9-9: MissingTemplateParam: BehaviorRegistry has missing template params when extending HasBehaviorsInterface, expecting 1 (see https://psalm.dev/182)
🔇 Additional comments (1)
src/product/behavior/TariffTypeBehaviorRegistry.php (1)
47-50: withBehaviors() return typing looks good.Signature and docblock align with the collection type and template.
1 participant
Summary by CodeRabbit
Refactor
Tests