-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
[π‘ FEATURE REQUEST]: Add getter for the PSR7Worker->httpWorker
property
#21
[π‘ FEATURE REQUEST]: Add getter for the PSR7Worker->httpWorker
property
#21
Conversation
WalkthroughThis update introduces an enhancement to the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration 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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/PSR7Worker.php (1 hunks)
Additional comments: 13
src/PSR7Worker.php (13)
- 57-60: The addition of the
getHttpWorker
method is straightforward and correctly implemented. It provides direct access to theHttpWorker
instance as intended by the PR objectives. This method is consistent with the class's design and follows proper PHP syntax and conventions.- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The file's declaration and namespace are correctly defined, ensuring that the
PSR7Worker
class is properly encapsulated within theSpiral\RoadRunner\Http
namespace. This is crucial for maintaining the package's structure and avoiding conflicts with classes from other namespaces.
- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [13-13]
The use of PHP 7.4's typed properties in the class definition enhances type safety and code readability. This modern PHP feature is well-utilized here, contributing to the overall maintainability and robustness of the code.
- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [33-33]
The constructor's use of dependency injection for
ServerRequestFactoryInterface
,StreamFactoryInterface
, andUploadedFileFactoryInterface
is a good practice. It adheres to the SOLID principles, specifically the Dependency Inversion Principle, by depending on abstractions rather than concrete implementations. This design choice facilitates easier testing and future modifications.
- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [36-36]
Initializing
HttpWorker
within the constructor and setting$_SERVER
to its original state ensures that thePSR7Worker
class has all necessary dependencies and context from the start. This approach is clean and avoids potential issues with uninitialized properties or context mismatches.
- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [43-43]
The
getWorker
method, which proxies the call toHttpWorker->getWorker()
, demonstrates good encapsulation practices. It allowsPSR7Worker
to expose necessary functionality of the wrappedHttpWorker
without breaking encapsulation. This method is a good example of the Facade design pattern, simplifying the interface for clients ofPSR7Worker
.
- 62-62: The
waitRequest
method's implementation, particularly its handling of null returns fromHttpWorker->waitRequest()
, is correctly done. It ensures that the method can gracefully handle cases where no request is available, returningnull
as expected. This is an important aspect of robust error handling in asynchronous or event-driven environments.- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [73-73]
The
respond
method's ability to handle both streaming and non-streaming responses based on thechunkSize
property is a flexible design choice. It allows for optimized data transfer based on the response size, which can be particularly beneficial in high-performance or resource-constrained environments.
- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [85-85]
The
streamToGenerator
method is an innovative use of PHP's generators for streaming response data. This method efficiently handles large responses by breaking them into manageable chunks, reducing memory usage. The implementation is correct and considers both fixed-size and unknown-size streams, ensuring compatibility with various response types.
- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [108-108]
The
configureServer
method's approach to reconstructing the$_SERVER
superglobal from the RoadRunner request object is thorough and well-implemented. It ensures that the PSR-7 request object created later in the process has a complete and accurate server environment, which is crucial for compatibility with applications expecting certain$_SERVER
values.
- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [138-138]
The
mapRequest
method's comprehensive mapping from the RoadRunner request to a PSR-7 request object is well-executed. It correctly handles all aspects of the request, including method, URI, headers, cookies, query parameters, uploaded files, and body content. This method is a key component of the class, enabling the seamless integration of RoadRunner with PSR-7 based applications.
- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [176-176]
The
wrapUploads
method's handling of uploaded files, converting them to instances ofUploadedFileInterface
, is correctly implemented. It ensures that files uploaded through RoadRunner requests are properly encapsulated in PSR-7 compliant objects, facilitating their handling in user applications. This method demonstrates attention to detail in adhering to PSR-7 standards.
- 54-64: > π NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [198-198]
The static method
fetchProtocolVersion
correctly normalizes HTTP protocol versions to valid values expected by PSR-7 implementations. This normalization is important for ensuring compatibility across different HTTP clients and servers, especially given the variations in protocol version notation.
Codecov ReportAll modified and coverable lines are covered by tests β
β Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## 3.x #21 +/- ##
============================================
+ Coverage 18.85% 21.46% +2.61%
- Complexity 60 61 +1
============================================
Files 4 4
Lines 175 177 +2
============================================
+ Hits 33 38 +5
+ Misses 142 139 -3 β View full report in Codecov by Sentry. |
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.
Without HttpWorker, we don't have the ability to use some specific functionality like early hints.
I think an access to a lower-level abstraction seems like a good solution. However, I would prefer to see all the capabilities of HttpWorker in PSR7Worker.
BTW @FluffyDiscord could you add some Unit Test to cover this getter? |
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.
lgtm
I guess I can |
Would the last commit do? I am not sure how you would like to create the test, should have asked. |
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.
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/Unit/PSR7WorkerTest.php (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/Unit/PSR7WorkerTest.php
Would it be possible to release new package version with this feature? |
Yep, will be released shortly. |
We are able to get the Spiral Worker from
Spiral\RoadRunner\Http\PSR7Worker
but for some reason we cannot get the wrappedHttpWorker
. This PR fixes that.Summary by CodeRabbit
PSR7Worker
class.