refactor(clickhouse): make retention a readonly constructor property#129
refactor(clickhouse): make retention a readonly constructor property#129levivannoort wants to merge 2 commits into
Conversation
Replace the getRetention/setRetention accessors with a promoted public readonly ?int constructor property. The positive-days check moves into the constructor body. Immutable-after-construction is coroutine-safe; the fluent setter is dropped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR converts the ClickHouse adapter's
Confidence Score: 1/5Not safe to merge — The PR leaves a public method (
Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/Audit/Adapter/ClickHouse.php:366-371
**`setRetention()` writes to a `readonly` property — PHP Fatal Error**
`$retention` is declared `public readonly ?int $retention` at line 102. PHP does not permit re-assignment of a `readonly` property after object construction; any call to `setRetention()` will throw `\Error: Cannot modify readonly property ClickHouse::$retention`. The PR description states "the fluent setter is dropped", but the method was left in. It needs to be removed (or the `readonly` modifier removed from the property if the setter is intentional).
### Issue 2 of 2
tests/Audit/Adapter/ClickHouseTest.php:407-420
**Test calls the broken setter — will always error before any assertion**
`testSetRetention` invokes `$adapter->setRetention(30)` on an already-constructed object whose `$retention` is `readonly`. This immediately throws `\Error: Cannot modify readonly property ClickHouse::$retention`, so none of the assertions below that line are ever reached. If `setRetention()` is removed (the PR's stated intent), this entire test block should be removed. If the setter is kept (and `readonly` dropped), the test is correct as-is.
Reviews (2): Last reviewed commit: "feat(clickhouse): make retention settabl..." | Re-trigger Greptile |
A single adapter instance runs setup() against several tables by mutating its namespace between calls (e.g. a shared `projects` table with a short window and a `console` table with a long one). A readonly, construction-time retention forced the same TTL onto every table. Drop `readonly`, add a fluent `setRetention()` so the window can be set per table before each setup(), and share validation via validateRetention(). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public function setRetention(?int $retention): self | ||
| { | ||
| return $this->asyncCleanup; | ||
| $this->validateRetention($retention); | ||
| $this->retention = $retention; | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
setRetention() writes to a readonly property — PHP Fatal Error
$retention is declared public readonly ?int $retention at line 102. PHP does not permit re-assignment of a readonly property after object construction; any call to setRetention() will throw \Error: Cannot modify readonly property ClickHouse::$retention. The PR description states "the fluent setter is dropped", but the method was left in. It needs to be removed (or the readonly modifier removed from the property if the setter is intentional).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Audit/Adapter/ClickHouse.php
Line: 366-371
Comment:
**`setRetention()` writes to a `readonly` property — PHP Fatal Error**
`$retention` is declared `public readonly ?int $retention` at line 102. PHP does not permit re-assignment of a `readonly` property after object construction; any call to `setRetention()` will throw `\Error: Cannot modify readonly property ClickHouse::$retention`. The PR description states "the fluent setter is dropped", but the method was left in. It needs to be removed (or the `readonly` modifier removed from the property if the setter is intentional).
How can I resolve this? If you propose a fix, please make it concise.
| username: 'default', | ||
| password: 'clickhouse' | ||
| ); | ||
|
|
||
| $adapter->setRetention(30); | ||
| $this->assertSame($adapter, $adapter->setRetention(30)); | ||
| $this->assertEquals(30, $adapter->retention); | ||
|
|
||
| $adapter->setRetention(null); | ||
| $this->assertNull($adapter->getRetention()); | ||
| $this->assertNull($adapter->retention); | ||
|
|
||
| $this->expectException(Exception::class); | ||
| $this->expectExceptionMessage('Retention must be a positive number of days'); | ||
| $adapter->setRetention(0); | ||
| } |
There was a problem hiding this comment.
testSetRetention invokes $adapter->setRetention(30) on an already-constructed object whose $retention is readonly. This immediately throws \Error: Cannot modify readonly property ClickHouse::$retention, so none of the assertions below that line are ever reached. If setRetention() is removed (the PR's stated intent), this entire test block should be removed. If the setter is kept (and readonly dropped), the test is correct as-is.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/Audit/Adapter/ClickHouseTest.php
Line: 407-420
Comment:
**Test calls the broken setter — will always error before any assertion**
`testSetRetention` invokes `$adapter->setRetention(30)` on an already-constructed object whose `$retention` is `readonly`. This immediately throws `\Error: Cannot modify readonly property ClickHouse::$retention`, so none of the assertions below that line are ever reached. If `setRetention()` is removed (the PR's stated intent), this entire test block should be removed. If the setter is kept (and `readonly` dropped), the test is correct as-is.
How can I resolve this? If you propose a fix, please make it concise.2 participants
What
Follow-up to #128. Converts the ClickHouse
retentionconfig from agetRetention()/setRetention()accessor pair to a promotedpublic readonly ?int $retentionconstructor property.Why
Readonly-after-construction is coroutine-safe — no mid-flight mutation of adapter config. Aligns with the style we're standardizing across codebases.
Notes
Internal
$this->retentionusage insetup()is unchanged. Retention tests now build the adapter via the constructor and assert on the property.🤖 Generated with Claude Code