Conversation
📝 WalkthroughWalkthroughAdds PHP strict typing across the repository by enabling Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2.x #474 +/- ##
=========================================
Coverage 98.72% 98.72%
Complexity 783 783
=========================================
Files 29 29
Lines 2275 2275
=========================================
Hits 2246 2246
Misses 29 29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Redmine/Api/CustomField.php`:
- Line 3: The listing() method currently forwards an untyped $forceUpdate to
doListing(bool $forceUpdate, ...) under declare(strict_types=1), causing a
TypeError when callers pass non-bool values; update listing() (in
CustomField.php) to coerce or validate $forceUpdate to a bool before calling
doListing — e.g., explicitly cast (bool)$forceUpdate or perform is_bool check
and convert — so the forwarded argument matches doListing(bool $forceUpdate,
...) and avoids strict-type failures.
In `@src/Redmine/Api/Search.php`:
- Line 3: The search() method currently forwards an untyped $query to
listByQuery(string $query, ...) under strict_types, causing a TypeError; fix
this by explicitly casting $query to string when calling listByQuery (e.g., pass
(string)$query) inside the search() implementation so the call matches
listByQuery's signature and avoids uncaught Error types; update any related call
sites in the same method (references: search(), listByQuery()) accordingly.
In `@src/Redmine/Serializer/XmlSerializer.php`:
- Line 80: The catch blocks in XmlSerializer (the catch clauses currently
written as "catch (Exception $e)") are too narrow and will let TypeError/other
Error subclasses escape under strict_types; update both catch clauses in the
XmlSerializer class (the blocks that currently catch Exception and wrap/rethrow
as SerializerException) to catch Throwable instead, preserving the existing
handling logic (wrap/throw SerializerException with the same message/context) so
TypeError and other Errors are captured and converted to SerializerException as
callers expect.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34594283-87b3-4479-a24f-4084a0dace48
📒 Files selected for processing (144)
.php-cs-fixer.dist.phpsrc/Redmine/Api.phpsrc/Redmine/Api/AbstractApi.phpsrc/Redmine/Api/Attachment.phpsrc/Redmine/Api/CustomField.phpsrc/Redmine/Api/Group.phpsrc/Redmine/Api/Issue.phpsrc/Redmine/Api/IssueCategory.phpsrc/Redmine/Api/IssuePriority.phpsrc/Redmine/Api/IssueRelation.phpsrc/Redmine/Api/IssueStatus.phpsrc/Redmine/Api/Membership.phpsrc/Redmine/Api/News.phpsrc/Redmine/Api/Project.phpsrc/Redmine/Api/Query.phpsrc/Redmine/Api/Role.phpsrc/Redmine/Api/Search.phpsrc/Redmine/Api/TimeEntry.phpsrc/Redmine/Api/TimeEntryActivity.phpsrc/Redmine/Api/Tracker.phpsrc/Redmine/Api/User.phpsrc/Redmine/Api/Version.phpsrc/Redmine/Api/Wiki.phpsrc/Redmine/Client/Client.phpsrc/Redmine/Client/ClientApiTrait.phpsrc/Redmine/Exception.phpsrc/Redmine/Exception/ClientException.phpsrc/Redmine/Exception/InvalidApiNameException.phpsrc/Redmine/Exception/InvalidParameterException.phpsrc/Redmine/Exception/MissingParameterException.phpsrc/Redmine/Exception/SerializerException.phpsrc/Redmine/Serializer/JsonSerializer.phpsrc/Redmine/Serializer/PathSerializer.phpsrc/Redmine/Serializer/XmlSerializer.phptests/Unit/Api/AbstractApiTest.phptests/Unit/Api/Attachment/DownloadTest.phptests/Unit/Api/Attachment/FromHttpClientTest.phptests/Unit/Api/Attachment/ShowTest.phptests/Unit/Api/Attachment/UpdateTest.phptests/Unit/Api/Attachment/UploadTest.phptests/Unit/Api/AttachmentTest.phptests/Unit/Api/CustomField/FromHttpClientTest.phptests/Unit/Api/CustomField/ListTest.phptests/Unit/Api/CustomFieldTest.phptests/Unit/Api/Group/AddUserTest.phptests/Unit/Api/Group/FromHttpClientTest.phptests/Unit/Api/Group/ListTest.phptests/Unit/Api/Group/ShowTest.phptests/Unit/Api/Issue/AddWatcherTest.phptests/Unit/Api/Issue/AttachManyTest.phptests/Unit/Api/Issue/AttachTest.phptests/Unit/Api/Issue/CreateTest.phptests/Unit/Api/Issue/FromHttpClientTest.phptests/Unit/Api/Issue/ListTest.phptests/Unit/Api/Issue/RemoveTest.phptests/Unit/Api/Issue/RemoveWatcherTest.phptests/Unit/Api/Issue/SetIssueStatusTest.phptests/Unit/Api/Issue/ShowTest.phptests/Unit/Api/IssueCategory/CreateTest.phptests/Unit/Api/IssueCategory/FromHttpClientTest.phptests/Unit/Api/IssueCategory/ListByProjectTest.phptests/Unit/Api/IssueCategory/RemoveTest.phptests/Unit/Api/IssueCategory/ShowTest.phptests/Unit/Api/IssueCategoryTest.phptests/Unit/Api/IssuePriority/FromHttpClientTest.phptests/Unit/Api/IssuePriority/ListTest.phptests/Unit/Api/IssuePriorityTest.phptests/Unit/Api/IssueRelation/CreateTest.phptests/Unit/Api/IssueRelation/FromHttpClientTest.phptests/Unit/Api/IssueRelation/ListByIssueIdTest.phptests/Unit/Api/IssueRelation/RemoveTest.phptests/Unit/Api/IssueRelation/ShowTest.phptests/Unit/Api/IssueRelationTest.phptests/Unit/Api/IssueStatus/FromHttpClientTest.phptests/Unit/Api/IssueStatus/ListTest.phptests/Unit/Api/IssueStatusTest.phptests/Unit/Api/IssueTest.phptests/Unit/Api/Membership/CreateTest.phptests/Unit/Api/Membership/FromHttpClientTest.phptests/Unit/Api/Membership/ListByProjectTest.phptests/Unit/Api/Membership/RemoveMemberTest.phptests/Unit/Api/Membership/RemoveTest.phptests/Unit/Api/Membership/UpdateTest.phptests/Unit/Api/MembershipTest.phptests/Unit/Api/News/FromHttpClientTest.phptests/Unit/Api/News/ListByProjectTest.phptests/Unit/Api/News/ListTest.phptests/Unit/Api/NewsTest.phptests/Unit/Api/Project/CloseTest.phptests/Unit/Api/Project/CreateTest.phptests/Unit/Api/Project/FromHttpClientTest.phptests/Unit/Api/Project/ListTest.phptests/Unit/Api/Project/RemoveTest.phptests/Unit/Api/Project/ReopenTest.phptests/Unit/Api/Project/ShowTest.phptests/Unit/Api/Project/UnarchiveTest.phptests/Unit/Api/ProjectTest.phptests/Unit/Api/Query/FromHttpClientTest.phptests/Unit/Api/Query/ListTest.phptests/Unit/Api/QueryTest.phptests/Unit/Api/Role/FromHttpClientTest.phptests/Unit/Api/Role/ListTest.phptests/Unit/Api/Role/ShowTest.phptests/Unit/Api/RoleTest.phptests/Unit/Api/Search/FromHttpClientTest.phptests/Unit/Api/Search/ListByQueryTest.phptests/Unit/Api/Search/SearchTest.phptests/Unit/Api/SearchTest.phptests/Unit/Api/TimeEntry/CreateTest.phptests/Unit/Api/TimeEntry/FromHttpClientTest.phptests/Unit/Api/TimeEntry/ListTest.phptests/Unit/Api/TimeEntry/RemoveTest.phptests/Unit/Api/TimeEntry/ShowTest.phptests/Unit/Api/TimeEntryActivity/FromHttpClientTest.phptests/Unit/Api/TimeEntryActivity/ListTest.phptests/Unit/Api/TimeEntryActivityTest.phptests/Unit/Api/TimeEntryTest.phptests/Unit/Api/Tracker/FromHttpClientTest.phptests/Unit/Api/Tracker/ListTest.phptests/Unit/Api/TrackerTest.phptests/Unit/Api/User/CreateTest.phptests/Unit/Api/User/FromHttpClientTest.phptests/Unit/Api/User/ListTest.phptests/Unit/Api/User/RemoveTest.phptests/Unit/Api/User/ShowTest.phptests/Unit/Api/UserTest.phptests/Unit/Api/Version/CreateTest.phptests/Unit/Api/Version/FromHttpClientTest.phptests/Unit/Api/Version/ListByProjectTest.phptests/Unit/Api/Version/RemoveTest.phptests/Unit/Api/Version/ShowTest.phptests/Unit/Api/Version/UpdateTest.phptests/Unit/Api/VersionTest.phptests/Unit/Api/Wiki/FromHttpClientTest.phptests/Unit/Api/Wiki/ListByProjectTest.phptests/Unit/Api/Wiki/ShowTest.phptests/Unit/Api/WikiTest.phptests/Unit/Client/NativeCurlClient/RequestTest.phptests/Unit/Client/Psr18Client/RequestTest.phptests/Unit/Client/Psr18ClientTest.phptests/Unit/Exception/ClientExceptionTest.phptests/Unit/Exception/InvalidApiNameExceptionTest.phptests/Unit/Exception/InvalidParameterExceptionTest.phptests/Unit/Exception/MissingParameterExceptionTest.php
| @@ -1,5 +1,7 @@ | |||
| <?php | |||
|
|
|||
| declare(strict_types=1); | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=php '\bpublic function search\(\$query, array \$params = \[\]\)|\blistByQuery\(\$query, \$params\)|catch \(Exception \$e\)' src/Redmine/Api/Search.phpRepository: kbsali/php-redmine-api
Length of output: 222
🏁 Script executed:
cd src/Redmine/Api && cat -n Search.php | head -100Repository: kbsali/php-redmine-api
Length of output: 4157
🏁 Script executed:
grep -n "public function listByQuery" src/Redmine/Api/Search.php -A 2Repository: kbsali/php-redmine-api
Length of output: 191
🏁 Script executed:
python3 << 'EOF'
# In PHP 7.0+, TypeError is a subclass of Error, not Exception
# So catch(Exception) would NOT catch TypeError
# Let's verify this understanding is correct
print("PHP Type Hierarchy (PHP 7+):")
print("- Error (base for all errors)")
print(" - TypeError (inherits from Error, NOT Exception)")
print("- Exception (separate from Error)")
print("\nConclusion: catch(Exception) will NOT catch TypeError")
EOFRepository: kbsali/php-redmine-api
Length of output: 264
Cast $query to string before passing to listByQuery().
With declare(strict_types=1);, the untyped $query parameter in search() will cause a TypeError when passed to listByQuery(string $query, ...), and this error won't be caught by catch (Exception $e) since TypeError extends Error, not Exception.
Fix
public function search($query, array $params = [])
{
`@trigger_error`('`' . __METHOD__ . '()` is deprecated since v2.4.0, use `' . self::class . '::listByQuery()` instead.', E_USER_DEPRECATED);
try {
- $results = $this->listByQuery($query, $params);
+ $results = $this->listByQuery((string) $query, $params);
} catch (Exception $e) {
if ($this->getLastResponse()->getContent() === '') {
return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Redmine/Api/Search.php` at line 3, The search() method currently forwards
an untyped $query to listByQuery(string $query, ...) under strict_types, causing
a TypeError; fix this by explicitly casting $query to string when calling
listByQuery (e.g., pass (string)$query) inside the search() implementation so
the call matches listByQuery's signature and avoids uncaught Error types; update
any related call sites in the same method (references: search(), listByQuery())
accordingly.
| try { | ||
| $this->deserialized = new SimpleXMLElement($encoded); | ||
| } catch (Throwable $e) { | ||
| } catch (Exception $e) { |
There was a problem hiding this comment.
Narrowing catch from Throwable to Exception may let TypeError escape uncaught.
With declare(strict_types=1), type mismatches now throw TypeError which extends Error, not Exception. This catch block will no longer capture:
TypeErrorfrom strict type violations during XML processing- Other
Errorsubclasses that were previously caught
Call sites like AbstractApi::getResponseAsArray() and various API create()/update() methods expect SerializerException but will now receive raw TypeError if caller-provided data has incorrect types.
Proposed fix: Keep catching Throwable
- } catch (Exception $e) {
+ } catch (\Throwable $e) {Apply to both catch blocks at lines 80 and 129.
Also applies to: 129-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Redmine/Serializer/XmlSerializer.php` at line 80, The catch blocks in
XmlSerializer (the catch clauses currently written as "catch (Exception $e)")
are too narrow and will let TypeError/other Error subclasses escape under
strict_types; update both catch clauses in the XmlSerializer class (the blocks
that currently catch Exception and wrap/rethrow as SerializerException) to catch
Throwable instead, preserving the existing handling logic (wrap/throw
SerializerException with the same message/context) so TypeError and other Errors
are captured and converted to SerializerException as callers expect.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 3 file(s) based on 3 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/Unit/Api/RoleTest.php (1)
258-259: Consider splitting this into a separate test case for clarity.The test now verifies two different scenarios:
listing(true)andlisting(1). While this validates that the$forceUpdateparameter correctly handles truthy int values (important given the untyped parameter inRole::listing()), mixing these cases in one test method slightly obscures the intent.If this is intentionally testing backward compatibility for non-bool inputs, a dedicated test method like
testListingAcceptsIntAsForceUpdate()would make the purpose clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Unit/Api/RoleTest.php` around lines 258 - 259, The current test mixes two scenarios (calling Role::listing() with a boolean and with an int) which reduces clarity; extract the assertion that calls $api->listing(1) into its own test method (e.g. testListingAcceptsIntAsForceUpdate) that sets up the same expected return and invokes listing(1) (retaining the // `@phpstan-ignore` comment if needed), and leave the original test to only assert listing(true); ensure both tests use the same mock/setup for $api and $expectedReturn so behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/Unit/Api/RoleTest.php`:
- Around line 258-259: The current test mixes two scenarios (calling
Role::listing() with a boolean and with an int) which reduces clarity; extract
the assertion that calls $api->listing(1) into its own test method (e.g.
testListingAcceptsIntAsForceUpdate) that sets up the same expected return and
invokes listing(1) (retaining the // `@phpstan-ignore` comment if needed), and
leave the original test to only assert listing(true); ensure both tests use the
same mock/setup for $api and $expectedReturn so behavior is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 004e6551-2925-4c29-953e-2c055c2024d7
📒 Files selected for processing (17)
src/Redmine/Api/CustomField.phpsrc/Redmine/Api/IssueCategory.phpsrc/Redmine/Api/IssueStatus.phpsrc/Redmine/Api/Project.phpsrc/Redmine/Api/Tracker.phpsrc/Redmine/Api/User.phpsrc/Redmine/Api/Version.phptests/Unit/Api/CustomFieldTest.phptests/Unit/Api/GroupTest.phptests/Unit/Api/IssueCategoryTest.phptests/Unit/Api/IssueStatusTest.phptests/Unit/Api/ProjectTest.phptests/Unit/Api/RoleTest.phptests/Unit/Api/TimeEntryActivityTest.phptests/Unit/Api/TrackerTest.phptests/Unit/Api/UserTest.phptests/Unit/Api/VersionTest.php
✅ Files skipped from review due to trivial changes (2)
- tests/Unit/Api/IssueStatusTest.php
- tests/Unit/Api/TimeEntryActivityTest.php
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/Unit/Api/ProjectTest.php
- src/Redmine/Api/Project.php
- tests/Unit/Api/CustomFieldTest.php
- tests/Unit/Api/VersionTest.php
- src/Redmine/Api/CustomField.php
- tests/Unit/Api/IssueCategoryTest.php
- tests/Unit/Api/TrackerTest.php
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
1 participant
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.
closes #464.
Summary by CodeRabbit