fix: Default PhpK8sException message parameter to empty string#501
fix: Default PhpK8sException message parameter to empty string#501dshafik wants to merge 1 commit into
Conversation
This avoids an E_DEPRECATED error when calling the `parent::__construct()` method. Alternatively, line 24 could be changed to `$message ?? ""`
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts PhpK8sException’s constructor default message to an empty string to avoid deprecation notices when delegating to the base Exception constructor. Class diagram for updated PhpK8sException constructorclassDiagram
class PhpK8sException {
<<exception>>
+__construct(message string = "", code int = 0, payload array?)
}
class Exception {
+__construct(message string = "", code int = 0)
}
PhpK8sException --|> Exception
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughSummaryThis change modifies the AssessmentWhat this fixes:
Backwards compatibility:
What's not addressed:
Realistic take: WalkthroughThe Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.1.54)PHPStan was skipped because the sandbox runner could not parse its output. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider using
$message ?? ""when callingparent::__construct()instead of changing the default argument value, so that existing callers that explicitly passnullretain their behavior while still avoiding the deprecation warning. - The docblock still documents
$messageasstring|null; if you intend to disallownullgoing forward, update it accordingly, otherwise keep accepting?stringand normalize only at theparent::__constructcall site.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using `$message ?? ""` when calling `parent::__construct()` instead of changing the default argument value, so that existing callers that explicitly pass `null` retain their behavior while still avoiding the deprecation warning.
- The docblock still documents `$message` as `string|null`; if you intend to disallow `null` going forward, update it accordingly, otherwise keep accepting `?string` and normalize only at the `parent::__construct` call site.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the PhpK8sException constructor to use an empty string as the default value for the message parameter to mitigate PHP deprecation warnings. The review feedback correctly identifies that this change does not cover cases where null is explicitly passed and suggests using the null coalescing operator in the parent constructor call to fully resolve the issue.
| * @param int $code | ||
| */ | ||
| public function __construct($message = null, $code = 0, ?array $payload = null) | ||
| public function __construct($message = "", $code = 0, ?array $payload = null) |
There was a problem hiding this comment.
Changing the default value to an empty string only prevents the deprecation error when the $message argument is omitted. If null is explicitly passed to the constructor (which is still permitted by the docblock and the lack of a type hint), the E_DEPRECATED error will still occur when parent::__construct($message, $code) is called on line 24.
To fully resolve this for all cases, it is recommended to implement the alternative mentioned in the PR description by using the null coalescing operator in the parent call: parent::__construct($message ?? "", $code);. This ensures a string is always passed to the base Exception class.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Exceptions/PhpK8sException.php`:
- Around line 22-24: Update the docblock for PhpK8sException::__construct to
reflect the new non-null default for $message: change the `@param` annotation from
"string|null $message" to "string $message" so the docblock matches the
constructor signature and no longer suggests null is accepted.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5bbdc13a-12a6-4e58-a0b8-82a142288b8a
📒 Files selected for processing (1)
src/Exceptions/PhpK8sException.php
| public function __construct($message = "", $code = 0, ?array $payload = null) | ||
| { | ||
| parent::__construct($message, $code); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all call sites of PhpK8sException constructor to check for explicit null being passed.
rg -n --type=php -C2 'new PhpK8sException\s*\(\s*null'Repository: renoki-co/php-k8s
Length of output: 43
🏁 Script executed:
# First, locate and inspect the PhpK8sException class to check the docblock
cat -n src/Exceptions/PhpK8sException.php | head -40Repository: renoki-co/php-k8s
Length of output: 1008
🏁 Script executed:
# Search for all instantiations of PhpK8sException (with or without null)
rg -n --type=php 'new PhpK8sException' -C 1Repository: renoki-co/php-k8s
Length of output: 43
🏁 Script executed:
# Also check if there are any other ways the exception is thrown or constructed
rg -n --type=php 'PhpK8sException(' -C 1Repository: renoki-co/php-k8s
Length of output: 144
Update the docblock to match the new default behavior.
The fix is correct and complete—no call sites in the codebase pass null explicitly to the constructor. However, the docblock on line 19 still declares @param string|null $message, which contradicts the new default of "" and suggests the parameter accepts null when the code will now reject it (passing null to parent::__construct would trigger E_DEPRECATED in PHP 8.1+).
Change the docblock from:
`@param` string|null $message
to:
`@param` string $message
This keeps the docblock honest—the parameter no longer accepts null in practice.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Exceptions/PhpK8sException.php` around lines 22 - 24, Update the docblock
for PhpK8sException::__construct to reflect the new non-null default for
$message: change the `@param` annotation from "string|null $message" to "string
$message" so the docblock matches the constructor signature and no longer
suggests null is accepted.
1 participant
This avoids an E_DEPRECATED error when calling the
parent::__construct()method.Alternatively, line 24 could be changed to
$message ?? ""Summary by Sourcery
Bug Fixes: