HP-2883: Implement Business Central bills import service#114
HP-2883: Implement Business Central bills import service#114VadymHrechukha wants to merge 2 commits intohiqdev:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds three new value objects (BillSource, BillTxn, BillReversesId) and extends Bill and BillInterface to store and expose optional source, txn, and reversesId fields, including constructor wiring and accessors (plus a setter for reversesId). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/bill/Bill.php (1)
260-268: Asymmetric mutation API: setter only forreversesId, not forsourceandtxn
setReversesId()allows post-construction mutation of$reversesId, but$sourceand$txnhave no equivalent setters. If mutable post-construction state is intentional only for the reversal use-case, a short inline comment explaining this would prevent confusion for future maintainers. Otherwise, consider making the API consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bill/Bill.php` around lines 260 - 268, The class exposes mutation for $reversesId via setReversesId but has getters getSource() and getTxn() without corresponding setters, creating an asymmetric API; either add setters setSource(BillSource $source): void and setTxn(BillTxn $txn): void (or nullable variants) to mirror setReversesId, or if immutability for source/txn is intentional, add a concise inline comment near the properties and the getSource()/getTxn() methods explaining that only reversesId is mutable post-construction and why (referencing $reversesId, setReversesId, $source, getSource, $txn, getTxn).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bill/Bill.php`:
- Around line 290-297: The setter setReversesId is missing the nullable type
hint documented in the docblock; change its signature to accept ?BillReversesId
(i.e. public function setReversesId(?BillReversesId $reversesId): void) so it
matches the constructor's BillReversesId $reversesId = null intent and prevents
arbitrary values, and ensure any necessary import/use for BillReversesId is
present and the docblock stays consistent.
In `@src/bill/BillReversesId.php`:
- Around line 9-19: The getId(): int return type is incompatible with the
protected $id (int|null) and the public __construct($id = null); either make the
return nullable by changing getId(): ?int to reflect the property, or prevent
null construction by making __construct private and add a static factory like
fromInt(int $id): self that sets $this->id; locate the class BillReversesId, the
$id property, the __construct and getId methods and apply one of these two fixes
consistently so types no longer conflict.
---
Nitpick comments:
In `@src/bill/Bill.php`:
- Around line 260-268: The class exposes mutation for $reversesId via
setReversesId but has getters getSource() and getTxn() without corresponding
setters, creating an asymmetric API; either add setters setSource(BillSource
$source): void and setTxn(BillTxn $txn): void (or nullable variants) to mirror
setReversesId, or if immutability for source/txn is intentional, add a concise
inline comment near the properties and the getSource()/getTxn() methods
explaining that only reversesId is mutable post-construction and why
(referencing $reversesId, setReversesId, $source, getSource, $txn, getTxn).
| /** | ||
| * @param BillReversesId|null $reversesId | ||
| * @return void | ||
| */ | ||
| public function setReversesId($reversesId): void | ||
| { | ||
| $this->reversesId = $reversesId; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
setReversesId parameter is untyped — add ?BillReversesId hint
The @param BillReversesId|null docblock documents the intended type, but the actual parameter declaration is untyped. This is inconsistent with the constructor signature (line 92: BillReversesId $reversesId = null) and silently accepts any value at runtime.
♻️ Proposed fix
- public function setReversesId($reversesId): void
+ public function setReversesId(?BillReversesId $reversesId): void
{
$this->reversesId = $reversesId;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bill/Bill.php` around lines 290 - 297, The setter setReversesId is
missing the nullable type hint documented in the docblock; change its signature
to accept ?BillReversesId (i.e. public function setReversesId(?BillReversesId
$reversesId): void) so it matches the constructor's BillReversesId $reversesId =
null intent and prevents arbitrary values, and ensure any necessary import/use
for BillReversesId is present and the docblock stays consistent.
| /** @var int|null */ | ||
| protected $id; | ||
|
|
||
| public function __construct($id = null) | ||
| { | ||
| $this->id = $id; | ||
| } | ||
|
|
||
| public function getId(): int | ||
| { | ||
| return $this->id; |
There was a problem hiding this comment.
getId(): int will throw TypeError when $id is null
The property is documented as int|null and the constructor defaults $id to null, so any instance created via new BillReversesId() or new BillReversesId(null) will throw a TypeError when getId() is called under declare(strict_types=1). The docblock and return type are contradictory.
Either make the return type nullable to match the property, or restrict the constructor so a null instance cannot be created:
🐛 Option A — fix the return type to match the nullable property
- public function getId(): int
+ public function getId(): ?int
{
return $this->id;
}🐛 Option B — restrict construction to the typed factory only
- public function __construct($id = null)
+ private function __construct(int $id)
{
$this->id = $id;
}
public function getId(): int
{
return $this->id;
}With a private constructor, callers are forced through fromInt(int $id) and a null state becomes impossible.
📝 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.
| /** @var int|null */ | |
| protected $id; | |
| public function __construct($id = null) | |
| { | |
| $this->id = $id; | |
| } | |
| public function getId(): int | |
| { | |
| return $this->id; | |
| /** `@var` int|null */ | |
| protected $id; | |
| public function __construct($id = null) | |
| { | |
| $this->id = $id; | |
| } | |
| public function getId(): ?int | |
| { | |
| return $this->id; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bill/BillReversesId.php` around lines 9 - 19, The getId(): int return
type is incompatible with the protected $id (int|null) and the public
__construct($id = null); either make the return nullable by changing getId():
?int to reflect the property, or prevent null construction by making __construct
private and add a static factory like fromInt(int $id): self that sets
$this->id; locate the class BillReversesId, the $id property, the __construct
and getId methods and apply one of these two fixes consistently so types no
longer conflict.
1 participant
Summary by CodeRabbit