Feature/parity - #67
Conversation
There was a problem hiding this comment.
Pull request overview
This PR brings API/behavior parity updates around Consul KV and Health endpoints, including capturing KV warning headers into WriteMeta, improving key-path handling, and expanding unit/integration coverage to lock in the new behaviors.
Changes:
- Add
WriteMeta::Warningsand populate it from theX-Consul-KV-Warningresponse header viaRequestResponse::buildWriteMeta(). - Enhance
KVClientwith separator support forKeys(), strict/lenient key path handling helpers, and boolean-valued responses forAcquire()/Release(). - Add new Health client unit/integration tests and extend KV tests for warnings, separators, and key normalization.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/WriteMetaTest.php | Adds unit coverage for WriteMeta warnings constructor/setter/getter behavior. |
| tests/Unit/KV/KVClientTest.php | Extends KV unit tests for separator query param, KV warning headers, leading-slash behavior, and Acquire/Release return shape. |
| tests/Unit/Health/HealthClientTest.php | Introduces unit tests for Health endpoints, including the updated Ingress() signature behavior and state validation. |
| tests/Integration/KV/KVClientTest.php | Updates integration tests to reflect valued bool responses for Acquire/Release and adds separator coverage for Keys(). |
| tests/Integration/Health/HealthClientTest.php | Adds end-to-end Health integration coverage including node/service checks and state validation. |
| src/WriteMeta.php | Adds Warnings field plus accessor/mutator, and extends constructor to accept variadic warnings. |
| src/PHPLib/RequestResponse.php | Populates WriteMeta warnings from the X-Consul-KV-Warning response header. |
| src/KV/KVClient.php | Adds key path helpers, adds Keys() separator support, changes Acquire/Release to valued bool responses, and normalizes leading slashes on read paths. |
| src/Health/HealthClient.php | Updates Ingress() to support both legacy and Go-style signatures; tightens allowed State() values. |
| src/Consul.php | Adds Consul::_headerConsulKVWarning constant for the KV warning header name. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/KV/KVClient.php:185
- Release() has the same backwards-incompatible return-shape/type change as Acquire() (now ValuedWriteBoolResponse with [bool, WriteMeta, Err] offsets). This will break callers expecting the previous
[$wm, $err]destructuring contract.
public function Release(KVPair $p, null|WriteOptions $opts = null): ValuedWriteBoolResponse
{
$r = $this->_newPutRequest($this->_writePath($p->Key), $p->Value, $opts);
$r->params->set('release', $p->Session);
if (0 !== $p->Flags) {
$r->params->set('flags', (string)$p->Flags);
}
$resp = $this->_requireOK($this->_do($r));
$ret = new ValuedWriteBoolResponse();
$this->_unmarshalResponse($resp, $ret);
return $ret;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/Unit/Event/EventClientTest.php:104
- EventClient::IDToIndex throws on 32-bit PHP (PHP_INT_SIZE < 8), but this test doesn't guard for that and will fail on 32-bit environments. Add a 64-bit skip like the sign-bit test does.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/Session/SessionClient.php:46
- SessionClient::Create() currently passes a null SessionEntry through to _create(), which results in a PUT request with no JSON body at all. Previously (and for CreateNoChecks) a default SessionEntry payload is built; sending an empty object payload is safer and consistent with other create calls.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 224 out of 224 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Status/StatusClient.php:34
- Removing the previously public
LeaderWithQueryOptions/PeersWithQueryOptionsmethods is a backward-incompatible API change for consumers still calling those method names. Consider keeping them as deprecated wrappers that forward to the newLeader/Peersmethods with optional opts.
No description provided.