-
-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
This PR fixes a few deprecation warnings thrown when used with PHP 8.4 #72
This PR fixes a few deprecation warnings thrown when used with PHP 8.4 #72
Conversation
WalkthroughThe pull request introduces nullable type declarations across several classes and interfaces, enhancing flexibility in parameter handling. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Queue.php (1)
Line range hint
93-95
: Remove redundant null check on$this->options
.The null check on
$this->options
is unnecessary since:
$this->options
is initialized in the constructor and is never null- The property is not marked as nullable in the class definition
- if ($this->options !== null && \method_exists($this->options, 'mergeOptional')) { + if (\method_exists($this->options, 'mergeOptional')) { /** @var OptionsInterface $options */ $options = $this->options->mergeOptional($options); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Consumer.php
(1 hunks)src/Queue.php
(2 hunks)src/QueueInterface.php
(1 hunks)src/Task/PreparedTask.php
(1 hunks)
🔇 Additional comments (6)
src/QueueInterface.php (1)
49-49
: LGTM! Explicit nullable type declaration added correctly.
The change from OptionsInterface $options = null
to ?OptionsInterface $options = null
properly addresses the PHP 8.4 deprecation warning while maintaining backward compatibility.
Let's verify all implementing classes have been updated:
✅ Verification successful
Implementation matches interface - change is properly synchronized
The implementing class Queue
correctly uses the same nullable type declaration ?OptionsInterface $options = null
as the interface. Additionally, other usages of this parameter type in the codebase (like in Jobs::create
) also follow the same pattern, showing consistency across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all classes implementing QueueInterface
ast-grep --pattern 'class $_ implements $$$QueueInterface$$$'
Length of output: 63
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find QueueInterface implementations
rg "implements.*QueueInterface" -l
Length of output: 50
Script:
#!/bin/bash
# Also search for class definitions that might have QueueInterface on a separate line
rg "class.*implements" -A 2
Length of output: 2854
Script:
#!/bin/bash
# Let's also check the actual interface definition to understand its namespace
rg "interface QueueInterface"
Length of output: 79
Script:
#!/bin/bash
# Let's check the implementation in Queue.php to verify if it matches the interface
rg "create.*OptionsInterface" src/Queue.php -A 3
Length of output: 50
Script:
#!/bin/bash
# Let's try to get the full create method implementation from Queue.php
ast-grep --pattern 'public function create($$$)' src/Queue.php
Length of output: 64
Script:
#!/bin/bash
# Also check the method signature directly
rg "public function create" src/Queue.php -A 1
Length of output: 98
Script:
#!/bin/bash
# Let's get more context around the create method implementation
rg "public function create" src/Queue.php -B 1 -A 5
Length of output: 305
Script:
#!/bin/bash
# Also check if there are any other files that might contain this method signature
rg "function create.*OptionsInterface" -A 2
Length of output: 388
src/Consumer.php (1)
41-42
: LGTM! Explicit nullable types with proper default handling.
The changes correctly address PHP 8.4 deprecation warnings by:
- Making nullable types explicit for both parameters
- Maintaining existing behavior with default implementations
src/Task/PreparedTask.php (1)
28-28
: LGTM! Explicit nullable type with proper fallback.
The change correctly addresses PHP 8.4 deprecation warnings while maintaining the existing behavior of defaulting to a new Options instance when null is provided.
src/Queue.php (3)
75-75
: LGTM! Explicit nullable type declaration maintains backward compatibility.
The change from OptionsInterface $options = null
to ?OptionsInterface $options = null
properly addresses the PHP 8.4 deprecation warning while preserving the existing behavior and compatibility with RoadRunner 1.x.
90-90
: LGTM! Explicit nullable type declaration is correctly applied.
The change from OptionsInterface $options = null
to ?OptionsInterface $options = null
properly addresses the PHP 8.4 deprecation warning.
75-75
: Verify consistent nullable type declarations across the codebase.
Let's ensure that all related interfaces and implementations have been updated with explicit nullable types for the $options
parameter.
Also applies to: 90-90
✅ Verification successful
Let me gather more information to verify the consistency of nullable type declarations.
Let me check the actual Queue class implementation to complete the verification.
Nullable type declarations are consistent across the codebase
The nullable type declarations for the $options
parameter are consistent between:
- The interface
QueueInterface
inwithDefaultOptions(?OptionsInterface $options)
andcreate(..., ?OptionsInterface $options = null)
- The implementation in
Queue
class for both methods - The constructor parameter in
Queue
class
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for method signatures that might need similar updates
ast-grep --pattern 'function $_(
$_,
$_,
OptionsInterface $options = null
)'
# Search for the updated interface definitions
rg "interface.*QueueInterface"
Length of output: 165
Script:
#!/bin/bash
# Search for method signatures with OptionsInterface parameter
rg "OptionsInterface.+\$options" -A 1
# Search specifically for Queue class implementations
ast-grep --pattern 'class $_ implements QueueInterface'
# Look for method signatures in the interface
cat src/QueueInterface.php
Length of output: 2065
Script:
#!/bin/bash
# Check the Queue class implementation
cat src/Queue.php
Length of output: 4716
@acelaya Thanks! |
When using this library with PHP 8.4, I noticed some deprecation warnings like this:
Deprecated: Spiral\RoadRunner\Jobs\Consumer::__construct(): Implicitly marking parameter $receivedTaskFactory as nullable is deprecated, the explicit nullable type must be used instead
This is due to this change https://wiki.php.net/rfc/deprecate-implicitly-nullable-types, in which initializing a method argument with value
null
is deprecated if the type is not explicitly nullable.Summary by CodeRabbit
New Features
Bug Fixes