HP-2676: Fix issues caused by HP-2581 release: tariff view error, failing CI jobs, and SinglePrice type error#105
Conversation
…ling CI jobs, and SinglePrice type error
WalkthroughThe BillingRegistryService updates exception throwing to use context-aware factory methods and adjusts two return type hints to use an imported Generator. Several exception classes now implement HasContextInterface and use the HasContext trait to carry contextual payloads. No core control flow or business logic changes are introduced. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Registry as BillingRegistryService
participant Repo as Internal Registries
Caller->>Registry: getBehavior(behaviorClassWrapper, type)
Registry->>Repo: resolve behavior class
alt class not found
Registry-->>Caller: throw InvalidBehaviorException.make(msg, {behavior})
else class found but invalid
Registry-->>Caller: throw InvalidBehaviorException.make(msg, {behavior})
else no matching behavior
Registry-->>Caller: throw BehaviorNotFoundException.make(msg, {behavior, type})
else success
Registry-->>Caller: Behavior
end
sequenceDiagram
autonumber
actor Caller
participant Registry as BillingRegistryService
participant Repo as Internal Registries
Caller->>Registry: getPriceTypeDefinitionByPriceTypeName(typeName)
Registry->>Repo: find price type definition
alt not found
Registry-->>Caller: throw PriceTypeDefinitionNotFoundException.make(msg, {type: typeName})
else found
Registry-->>Caller: PriceTypeDefinition
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/product/Application/BillingRegistryService.php (2)
122-139: Guard against invalid behavior class to avoid fatal instanceof.
Without validation, passing an unknown class/interface triggers a fatal error during instanceof.Apply this diff to add validation at the start:
public function getBehaviors(string $behaviorClassWrapper): Generator { + if (!class_exists($behaviorClassWrapper) && !interface_exists($behaviorClassWrapper)) { + throw InvalidBehaviorException::make('Behavior class does not exist', [ + 'behavior' => $behaviorClassWrapper, + ]); + } + if (!is_subclass_of($behaviorClassWrapper, BehaviorInterface::class)) { + throw InvalidBehaviorException::make('Behavior class does not implement BehaviorInterface', [ + 'behavior' => $behaviorClassWrapper, + ]); + }If you prefer DRY, I can provide a private assertValidBehaviorClassWrapper(...) helper and wire both methods to it.
26-53: Ensure a static make() factory exists on all exception classesThe service now calls
InvalidRepresentationException::make(…)andInvalidBehaviorException::make(…), but neither these classes nor any shared trait define astatic make()method. You must add a static factory method—either via a newHasContexttrait or directly on each exception—to avoid fatal “undefined method” errors.• Update each exception under
src/productthat implementsHasContextInterface:
- src/product/invoice/InvalidRepresentationException.php
- src/product/behavior/InvalidBehaviorException.php
- src/product/behavior/BehaviorNotFoundException.php
- src/product/Exception/TariffTypeDefinitionNotFoundException.php
- src/product/Exception/PriceTypeDefinitionNotFoundException.php
• Add a trait at, for example,
src/Product/Exception/Traits/HasContext.php:<?php namespace Product\Exception\Traits; trait HasContext { public static function make(string $message, array $context = []): static { $exception = new static($message); if (method_exists($exception, 'setContext')) { $exception->setContext($context); } return $exception; } }• Then use it in each exception:
-class InvalidRepresentationException extends InvalidArgumentException implements HasContextInterface +use Product\Exception\Traits\HasContext; + +class InvalidRepresentationException extends InvalidArgumentException implements HasContextInterface +{ + use HasContext;This will restore the expected
::make()calls and centralize context handling.
🧹 Nitpick comments (4)
src/product/invoice/InvalidRepresentationException.php (1)
9-11: Optional: make the exception final.Domain exception classes are typically not extended; marking them final avoids inheritance-based surprises.
Apply this diff:
-class InvalidRepresentationException extends InvalidArgumentException implements HasContextInterface +final class InvalidRepresentationException extends InvalidArgumentException implements HasContextInterfacesrc/product/behavior/InvalidBehaviorException.php (1)
9-11: Optional: mark as final.Same rationale as other domain exceptions.
-class InvalidBehaviorException extends InvalidArgumentException implements HasContextInterface +final class InvalidBehaviorException extends InvalidArgumentException implements HasContextInterfacesrc/product/Exception/TariffTypeDefinitionNotFoundException.php (1)
9-11: Optional: reduce repetition via a contextual base exception.To avoid repeating “implements HasContextInterface” + “use HasContext;” in multiple exceptions, introduce a ContextRuntimeException and extend it here and in sibling exceptions. Keeps the pattern DRY and consistent.
Add a new base (outside the shown ranges):
<?php declare(strict_types=1); namespace hiqdev\php\billing\Exception; use hidev\exception\HasContext; use hidev\exception\HasContextInterface; abstract class ContextRuntimeException extends RuntimeException implements HasContextInterface { use HasContext; }Then change this file as follows:
-use hiqdev\php\billing\Exception\RuntimeException; +use hiqdev\php\billing\Exception\ContextRuntimeException; @@ -class TariffTypeDefinitionNotFoundException extends RuntimeException implements HasContextInterface -{ - use HasContext; -} +final class TariffTypeDefinitionNotFoundException extends ContextRuntimeException {}If you prefer to keep the current approach, at least consider marking the class final:
-class TariffTypeDefinitionNotFoundException extends RuntimeException implements HasContextInterface +final class TariffTypeDefinitionNotFoundException extends RuntimeException implements HasContextInterfacesrc/product/Exception/PriceTypeDefinitionNotFoundException.php (1)
9-11: Optional: align with DRY base or mark final.Either extend the proposed ContextRuntimeException or mark this class final to match the domain-exception style.
-class PriceTypeDefinitionNotFoundException extends RuntimeException implements HasContextInterface +final class PriceTypeDefinitionNotFoundException extends RuntimeException implements HasContextInterface
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/product/Application/BillingRegistryService.php(6 hunks)src/product/Exception/PriceTypeDefinitionNotFoundException.php(1 hunks)src/product/Exception/TariffTypeDefinitionNotFoundException.php(1 hunks)src/product/behavior/BehaviorNotFoundException.php(1 hunks)src/product/behavior/InvalidBehaviorException.php(1 hunks)src/product/invoice/InvalidRepresentationException.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/Application/BillingRegistryService.php
🧬 Code graph analysis (4)
src/product/Exception/PriceTypeDefinitionNotFoundException.php (1)
src/Exception/RuntimeException.php (1)
RuntimeException(6-9)
src/product/Application/BillingRegistryService.php (6)
src/product/invoice/InvalidRepresentationException.php (1)
InvalidRepresentationException(9-12)src/product/behavior/InvalidBehaviorException.php (1)
InvalidBehaviorException(9-12)src/product/Exception/TariffTypeDefinitionNotFoundException.php (1)
TariffTypeDefinitionNotFoundException(9-12)src/product/Application/BillingRegistryServiceInterface.php (3)
getBehavior(52-52)getBehaviors(61-61)findPriceTypeDefinitionsByBehavior(69-69)src/product/behavior/BehaviorNotFoundException.php (1)
BehaviorNotFoundException(9-12)src/product/Exception/PriceTypeDefinitionNotFoundException.php (1)
PriceTypeDefinitionNotFoundException(9-12)
src/product/behavior/BehaviorNotFoundException.php (1)
src/Exception/RuntimeException.php (1)
RuntimeException(6-9)
src/product/Exception/TariffTypeDefinitionNotFoundException.php (1)
src/Exception/RuntimeException.php (1)
RuntimeException(6-9)
🔇 Additional comments (13)
src/product/invoice/InvalidRepresentationException.php (1)
5-11: Context-aware exception wiring looks good.Interface + trait are correctly imported and applied; this aligns with context-rich errors across the PR.
src/product/behavior/InvalidBehaviorException.php (1)
5-11: Looks consistent with the new context pattern.Clean adoption of HasContext without behavior changes.
src/product/Exception/TariffTypeDefinitionNotFoundException.php (1)
5-11: Good move to carry context in not-found errors.Matches RuntimeException hierarchy and prepares for factory-based construction.
src/product/Exception/PriceTypeDefinitionNotFoundException.php (1)
5-11: Consistent context integration.Implementation mirrors TariffType* and will help diagnostics.
src/product/behavior/BehaviorNotFoundException.php (1)
5-11: Good: context added to a frequently thrown not-found error.This will surface payloads for registry lookups.
src/product/Application/BillingRegistryService.php (8)
5-5: Importing Generator to align with interface — LGTM.
Matches the interface signature; removes leading backslash noise.
29-31: Good: context-rich exception for missing representation.
Appropriate exception type and payload.
63-65: Good: not-found exception carries context.
Clear message + payload.
156-158: Good: price type not-found exception with payload.
Consistent with the rest.
20-24: Dependency boundary respected — LGTM.
Uses the internal BillingRegistryInterface; aligns with our learning to avoid advancedhosters/billing-registry.
71-75: Allow interfaces for behaviors in existence checkApply the following change in
src/product/Application/BillingRegistryService.phpat lines 71–75 (and similarly at 78–82) to ensure that interfaces are accepted alongside classes:- if (!class_exists($behaviorClassWrapper)) { + if (!class_exists($behaviorClassWrapper) && !interface_exists($behaviorClassWrapper)) { throw InvalidBehaviorException::make('Behavior class does not exist', [ 'behavior' => $behaviorClassWrapper, ]); }
- Note: the subsequent
is_subclass_of(...)call already supports interfaces, so no further updates are required there.I attempted to locate any existing callers passing interface names to
getBehavior(...)but did not find any matches in the codebase. Please manually verify that no behavior interfaces are currently used, to avoid introducing unintended breaking changes.
97-101: Confirmed PHP ≥ 7.3 support—no change needed
The project’s composer.json specifies"php": "^8.3", so trailing commas in function calls have been supported since PHP 7.3. You can safely keep the existing code as is.
161-168: I wasn’t able to locate the existing guard in getBehaviors (or verify its absence) via automated search. Could you please double-check that:• In BillingRegistryService::getBehaviors you’ve already added the class_exists / is_subclass_of guard;
• And if so, confirm its exact implementation so we can mirror it correctly here.Once confirmed, we can safely apply the same validation at the top of findPriceTypeDefinitionsByBehavior to prevent a fatal instanceof.
| throw InvalidBehaviorException::make('Representation class does not implement RepresentationInterface', [ | ||
| 'representationClass' => $representationClass, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Wrong exception type for representation contract violation.
Should use InvalidRepresentationException, not InvalidBehaviorException.
Apply this diff:
- throw InvalidBehaviorException::make('Representation class does not implement RepresentationInterface', [
+ throw InvalidRepresentationException::make('Representation class does not implement RepresentationInterface', [
'representationClass' => $representationClass,
]
);Optional hardening (outside selected lines): validate interfaces too, not only classes:
// replace the preceding condition with:
if ((class_exists($representationClass) || interface_exists($representationClass))
&& !is_subclass_of($representationClass, RepresentationInterface::class)
) {
throw InvalidRepresentationException::make(
'Representation class does not implement RepresentationInterface',
['representationClass' => $representationClass]
);
}🤖 Prompt for AI Agents
In src/product/Application/BillingRegistryService.php around lines 37 to 40, the
code throws InvalidBehaviorException for a representation contract violation;
replace that with InvalidRepresentationException and update the thrown call
parameters accordingly. Also (optional/hardening) adjust the condition that
checks the representation type to allow interfaces as well as classes by
checking class_exists OR interface_exists and then confirming the type
implements/extends RepresentationInterface before throwing
InvalidRepresentationException with the same message and representationClass
context.
| class BehaviorNotFoundException extends RuntimeException implements HasContextInterface | ||
| { | ||
| use HasContext; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verification: ensure the trait/interface and factory usage are wired in the repo.
- Confirm hidev/exception is required.
- Ensure callers consistently use static factories (e.g., ::make(...)) and no lingering direct “new …Exception(...)” remain if you intend uniform payloads.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Check hidev/exception dependency"
python - <<'PY'
import json,sys,os
p='composer.json'
if not os.path.exists(p):
print('composer.json not found'); sys.exit(0)
d=json.load(open(p))
req = {k.lower():v for k,v in d.get('require',{}).items()}
dev = {k.lower():v for k,v in d.get('require-dev',{}).items()}
for sect,name in ((req,'require'), (dev,'require-dev')):
if 'hidev/exception' in sect:
print(f'Found hidev/exception in {name}:', sect['hidev/exception']); break
else:
print('MISSING hidev/exception in require/require-dev')
PY
echo -e "\n2) Call sites using ::make(...)"
rg -nP --no-heading -C1 '\b(InvalidRepresentationException|TariffTypeDefinitionNotFoundException|PriceTypeDefinitionNotFoundException|InvalidBehaviorException|BehaviorNotFoundException)::make\s*\(' || true
echo -e "\n3) Direct instantiations (review and convert to factories if desired)"
rg -nP --no-heading -C1 '\bnew\s+(InvalidRepresentationException|TariffTypeDefinitionNotFoundException|PriceTypeDefinitionNotFoundException|InvalidBehaviorException|BehaviorNotFoundException)\s*\(' || trueLength of output: 3454
Add hidev/exception dependency and unify exception instantiation
The HasContext trait won’t work without the hidev/exception package, and there’s still one direct new …Exception() call that should be converted to use the static factory.
• composer.json
– Currently missing hidev/exception in both require and require-dev. Add the package (e.g. "hidev/exception": "^<version>") to the appropriate section.
• Exception instantiation in src/product/invoice/Representation.php:19
– Replace
php throw new InvalidRepresentationException('Representation SQL cannot be empty.');
with
php throw InvalidRepresentationException::make('Representation SQL cannot be empty.');
• BehaviorNotFoundException usage
– Call sites (e.g. in BillingRegistryService) correctly use BehaviorNotFoundException::make(...); no direct new BehaviorNotFoundException() instances remain.
🤖 Prompt for AI Agents
In src/product/behavior/BehaviorNotFoundException.php around lines 9-11, the
class uses the HasContext trait which requires the hidev/exception package; add
"hidev/exception":"^<version>" to composer.json (in require or require-dev as
appropriate) and run composer update so HasContext methods are available; in
src/product/invoice/Representation.php line 19 replace the direct instantiation
throw new InvalidRepresentationException('Representation SQL cannot be empty.');
with the static factory throw
InvalidRepresentationException::make('Representation SQL cannot be empty.'); and
audit the codebase to ensure no other direct new ...Exception() calls remain for
exceptions that provide a ::make factory (e.g., BehaviorNotFoundException),
converting them to the static factory where found.
2 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores