add mixed return type #727
Conversation
|
Would it work to resolve deprecation adding only phpdoc return typehint? |
|
Probably. I see that 'mixed' is PHP 8 only. PHP 7.4 reached EOL in 2022. What about simply bumping a new release and taking advantage of all the PHP 8 goodness? |
WalkthroughThe Changes
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
src/Contracts/Data.php (1)
32-39: Consider implementing null coalescing operatorThe method implementation could be simplified using PHP's null coalescing operator.
public function offsetGet($offset): mixed { - if (isset($this->data[$offset])) { - return $this->data[$offset]; - } - - return null; + return $this->data[$offset] ?? null; }This is a minor readability improvement that doesn't change functionality.
| #[\ReturnTypeWillChange] | ||
| public function offsetGet($offset) | ||
| public function offsetGet($offset): mixed |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant attribute with mixed return type
The change to add : mixed return type is correct and aligns with PHP 8 practices. However, the #[\ReturnTypeWillChange] attribute on line 31 is now redundant and should be removed since you've explicitly declared the return type.
- #[\ReturnTypeWillChange]
public function offsetGet($offset): mixedThis change appropriately resolves the deprecation notice while taking advantage of PHP 8's type system. As noted in the PR discussion, this does make PHP 8.0+ a requirement for this library.
📝 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.
| #[\ReturnTypeWillChange] | |
| public function offsetGet($offset) | |
| public function offsetGet($offset): mixed | |
| public function offsetGet($offset): mixed |
|
We can pick this up after merging #735 which drops PHP versions < 8. |
Pull Request
Related issue
Fixes #726
What does this PR do?
Gets rid of the deprecation notice. But I forget when 'mixed' was introduced and if this library is supporting a version of php that doesn't allow this.
Summary by CodeRabbit