refactor: throw typed Elastica exceptions for unsupported features#2303
Open
ruflin wants to merge 1 commit into
Open
refactor: throw typed Elastica exceptions for unsupported features#2303ruflin wants to merge 1 commit into
ruflin wants to merge 1 commit into
Conversation
Replace `throw new \Exception('Not supported')` calls in
`Elastica\Client` (setAsync/getAsync/setResponseException/
getResponseException/setServerless) and the bare `\Exception` in
`Elastica\Task::cancel()` with the existing typed exceptions:
- `Elastica\Exception\NotImplementedException` for unsupported features
on the Client. It already extends `\BadMethodCallException` and
implements `Elastica\Exception\ExceptionInterface`.
- `Elastica\Exception\InvalidException` for the missing-id guard in
`Task::cancel()`.
Callers can now `catch (Elastica\Exception\ExceptionInterface)` instead
of relying on the SPL root.
Update unit tests accordingly and add coverage that asserts the
NotImplementedException is catchable as both `BadMethodCallException`
and `ExceptionInterface`.
📝 WalkthroughWalkthroughThe pull request replaces generic exception throws with typed exceptions in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 4/8 reviews remaining, refill in 28 minutes and 24 seconds.Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors unsupported-feature code paths in Elastica\Client and Elastica\Task to throw existing typed Elastica exceptions instead of generic \Exception, improving catchability via Elastica\Exception\ExceptionInterface.
Changes:
Elastica\Clientunsupported feature methods now throwElastica\Exception\NotImplementedException.Elastica\Task::cancel()now throwsElastica\Exception\InvalidExceptionwhen called without a task id.- Tests and changelog updated to reflect the new typed exceptions.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/Client.php |
Replace generic exceptions with NotImplementedException for unsupported client features. |
src/Task.php |
Replace generic exception with InvalidException for missing task id; update @throws docs. |
tests/ClientTest.php |
Add data-provider coverage and interface-catchability assertions for NotImplementedException. |
tests/TaskTest.php |
Tighten expectation to InvalidException for empty task id cancel. |
CHANGELOG.md |
Document the exception type changes under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+259
to
+275
| * @return iterable<string, array{0: callable(Client): mixed}> | ||
| */ | ||
| public static function unsupportedClientFeaturesProvider(): iterable | ||
| { | ||
| yield 'setAsync' => [static fn (Client $client) => $client->setAsync(true)]; | ||
| yield 'getAsync' => [static fn (Client $client) => $client->getAsync()]; | ||
| yield 'setResponseException' => [static fn (Client $client) => $client->setResponseException(true)]; | ||
| yield 'getResponseException' => [static fn (Client $client) => $client->getResponseException()]; | ||
| yield 'setServerless' => [static fn (Client $client) => $client->setServerless(true)]; | ||
| } | ||
|
|
||
| #[DataProvider('unsupportedClientFeaturesProvider')] | ||
| public function testUnsupportedFeaturesThrowNotImplementedException(callable $invocation): void | ||
| { | ||
| $client = new Client(); | ||
|
|
||
| $this->expectException(NotImplementedException::class); |
|
|
||
| /** | ||
| * @throws \Exception | ||
| * @throws InvalidException if no task id is set |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/ClientTest.php`:
- Around line 257-278: The legacy single-case tests that still assert the old
message "Not supported" must be removed or updated to match the new
provider-based expectation: replace the message-based assertions with a typed
exception check (expectException(NotImplementedException::class)) or delete
those legacy tests so the provider-driven test
(unsupportedClientFeaturesProvider +
testUnsupportedFeaturesThrowNotImplementedException) is the single source of
truth; ensure references to Client methods (setAsync, getAsync,
setResponseException, getResponseException, setServerless) remain covered by the
provider test and that no remaining tests assert the exact "Not supported"
message.
- Around line 287-290: The test currently catches ExceptionInterface and asserts
it's a NotImplementedException then redundantly asserts \BadMethodCallException
(which PHPStan flags as always-true). Fix by making the catch specific (catch
NotImplementedException $e) or keep the generic catch but remove the redundant
$this->assertInstanceOf(\BadMethodCallException, $e);—ensure only a single
meaningful assertion remains referencing NotImplementedException (and retain
ExceptionInterface only if you need to assert the interface explicitly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a050cc3-02d4-4748-bd07-326f98bab7b4
📒 Files selected for processing (5)
CHANGELOG.mdsrc/Client.phpsrc/Task.phptests/ClientTest.phptests/TaskTest.php
Comment on lines
+257
to
+278
|
|
||
| /** | ||
| * @return iterable<string, array{0: callable(Client): mixed}> | ||
| */ | ||
| public static function unsupportedClientFeaturesProvider(): iterable | ||
| { | ||
| yield 'setAsync' => [static fn (Client $client) => $client->setAsync(true)]; | ||
| yield 'getAsync' => [static fn (Client $client) => $client->getAsync()]; | ||
| yield 'setResponseException' => [static fn (Client $client) => $client->setResponseException(true)]; | ||
| yield 'getResponseException' => [static fn (Client $client) => $client->getResponseException()]; | ||
| yield 'setServerless' => [static fn (Client $client) => $client->setServerless(true)]; | ||
| } | ||
|
|
||
| #[DataProvider('unsupportedClientFeaturesProvider')] | ||
| public function testUnsupportedFeaturesThrowNotImplementedException(callable $invocation): void | ||
| { | ||
| $client = new Client(); | ||
|
|
||
| $this->expectException(NotImplementedException::class); | ||
|
|
||
| $invocation($client); | ||
| } |
There was a problem hiding this comment.
Consolidate old unsupported-feature tests to the new typed-exception expectations.
This new provider-based test is good, but the legacy tests at Line 103, Line 120, Line 129, and Line 138 still assert the old message (Not supported). With the new messages from Client, those checks are now inconsistent and can fail CI. Please update/remove those legacy expectations and keep this provider-driven test as the single source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ClientTest.php` around lines 257 - 278, The legacy single-case tests
that still assert the old message "Not supported" must be removed or updated to
match the new provider-based expectation: replace the message-based assertions
with a typed exception check (expectException(NotImplementedException::class))
or delete those legacy tests so the provider-driven test
(unsupportedClientFeaturesProvider +
testUnsupportedFeaturesThrowNotImplementedException) is the single source of
truth; ensure references to Client methods (setAsync, getAsync,
setResponseException, getResponseException, setServerless) remain covered by the
provider test and that no remaining tests assert the exact "Not supported"
message.
Comment on lines
+287
to
+290
| } catch (ExceptionInterface $e) { | ||
| $this->assertInstanceOf(NotImplementedException::class, $e); | ||
| $this->assertInstanceOf(\BadMethodCallException::class, $e); | ||
| } |
There was a problem hiding this comment.
Remove the always-true assertion flagged by PHPStan.
After asserting NotImplementedException, the \BadMethodCallException assertion is redundant and reported as always true. Rework the catch/assert pattern to avoid tautological checks.
Suggested adjustment
- } catch (ExceptionInterface $e) {
- $this->assertInstanceOf(NotImplementedException::class, $e);
- $this->assertInstanceOf(\BadMethodCallException::class, $e);
+ } catch (\BadMethodCallException $e) {
+ $this->assertInstanceOf(ExceptionInterface::class, $e);
+ $this->assertInstanceOf(NotImplementedException::class, $e);
}📝 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.
Suggested change
| } catch (ExceptionInterface $e) { | |
| $this->assertInstanceOf(NotImplementedException::class, $e); | |
| $this->assertInstanceOf(\BadMethodCallException::class, $e); | |
| } | |
| } catch (\BadMethodCallException $e) { | |
| $this->assertInstanceOf(ExceptionInterface::class, $e); | |
| $this->assertInstanceOf(NotImplementedException::class, $e); | |
| } |
🧰 Tools
🪛 GitHub Check: PHPStan
[failure] 289-289:
Call to method PHPUnit\Framework\Assert::assertInstanceOf() with 'BadMethodCallException' and Elastica\Exception\NotImplementedException will always evaluate to true.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ClientTest.php` around lines 287 - 290, The test currently catches
ExceptionInterface and asserts it's a NotImplementedException then redundantly
asserts \BadMethodCallException (which PHPStan flags as always-true). Fix by
making the catch specific (catch NotImplementedException $e) or keep the generic
catch but remove the redundant $this->assertInstanceOf(\BadMethodCallException,
$e);—ensure only a single meaningful assertion remains referencing
NotImplementedException (and retain ExceptionInterface only if you need to
assert the interface explicitly).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replace
throw new \Exception('Not supported')calls inElastica\Clientand
Elastica\Taskwith the existing typed Elastica exceptions:Elastica\Client::setAsync(),getAsync(),setResponseException(),getResponseException()andsetServerless()now throwElastica\Exception\NotImplementedException(already extends\BadMethodCallExceptionand implementsElastica\Exception\ExceptionInterface).Elastica\Task::cancel()'s missing-id guard now throwsElastica\Exception\InvalidException.This lets callers catch a typed exception (or the
Elastica\Exception\ExceptionInterfaceumbrella) instead of relying onthe SPL root.
Identified during a P0/P1 code-review pass.
Test plan
DataProvidercovers all five Client methods.ExceptionInterfaceand\BadMethodCallException.TaskTest::testCancelThrowsExceptionWithEmptyTaskIdtightened to expect
InvalidException.Summary by CodeRabbit
Refactor
Tests