Skip to content
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

Stream & Composite handlers #95

Merged
merged 44 commits into from Oct 8, 2023
Merged

Stream & Composite handlers #95

merged 44 commits into from Oct 8, 2023

Conversation

xepozz
Copy link
Contributor

@xepozz xepozz commented Sep 4, 2023

Q A
Is bugfix?
New feature? ✔️
Breaks BC?

Simple listen udp server

<?php
$socket = socket_create(AF_INET, SOCK_DGRAM, SOL_UDP);
socket_bind($socket, "0.0.0.0", 8890);
$clients = [];
while (true){
    socket_recvfrom($socket, $buffer, 32768, 0, $ip, $port);
    var_dump('Received:', $buffer);
}

Simple udp write client

<?php
$socket = socket_create(AF_INET, SOCK_DGRAM, SOL_UDP);
$data = 'test';
socket_sendto($socket, $data, strlen($data), 0, '127.0.0.1', 8890);

@what-the-diff
Copy link

what-the-diff bot commented Sep 4, 2023

PR Summary

  • Introduction of new 'Output Destination' in README
    A new section has been added to the README explaining various classes to handle where the data "dumps" are sent. New classes including EchoHandler, UdpHandler, and CompositeHandler have been added to make this process more efficient.

  • Suggestion for use of 'ext-sockets' in composer.json
    An addition has been made in the 'suggest' area of our project's composer.json which recommends using the 'ext-sockets' extension. This can facilitate sending dumps to a server via the UDP protocol which can increase speed and performance.

  • New Feature: CompositeHandler
    A new feature, located in the src/Handler directory, called CompositeHandler has been introduced. This gives us the ability to send the dump data to multiple handlers simultaneously, increasing redundancy and reliability.

  • New Feature: UdpHandler
    Another new feature, titled UdpHandler, has also been created in the src/Handler directory. This class is equipped to send the dumps to a UDP socket, offering an additional method of data transmission.

@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f4f8b37) 91.82% compared to head (9fd803d) 92.39%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master      #95      +/-   ##
============================================
+ Coverage     91.82%   92.39%   +0.57%     
- Complexity      162      189      +27     
============================================
  Files             5        7       +2     
  Lines           379      434      +55     
============================================
+ Hits            348      401      +53     
- Misses           31       33       +2     
Files Coverage Δ
src/Handler/CompositeHandler.php 100.00% <100.00%> (ø)
src/Handler/StreamHandler.php 96.00% <96.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/Handler/UdpHandler.php Outdated Show resolved Hide resolved
@xepozz xepozz changed the title UDP & Composite handlers Stream & Composite handlers Sep 9, 2023
@xepozz xepozz requested a review from a team September 9, 2023 04:06
@xepozz xepozz added the status:code review The pull request needs review. label Sep 9, 2023
src/Handler/StreamHandler.php Outdated Show resolved Hide resolved
src/Handler/StreamHandler.php Outdated Show resolved Hide resolved
src/Handler/StreamHandler.php Show resolved Hide resolved
@arogachev arogachev self-requested a review September 12, 2023 05:59
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Handler/CompositeHandler.php Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
tests/Handler/StreamHandlerTest.php Outdated Show resolved Hide resolved
tests/Support/InMemoryHandler.php Show resolved Hide resolved
src/Handler/StreamHandler.php Outdated Show resolved Hide resolved
src/Handler/StreamHandler.php Show resolved Hide resolved
src/Handler/StreamHandler.php Outdated Show resolved Hide resolved
@arogachev arogachev mentioned this pull request Sep 18, 2023

/** @psalm-suppress PossiblyNullArgument */
if (@fwrite($this->stream, $data) === false) {
throw new RuntimeException('Cannot write a stream.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to reproduce async behaviour in tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#98

Copy link
Contributor

@arogachev arogachev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check #96.

* Corrections for PR number 95

* Fix Psalm

* Refactor

* More refactor

* Partial revert

* Move write to stream logic to a separate method

* Use a dedicated function for getting socket protocols

* Review fix related with requiring sockets php extension

* Use less variables (code review fix)

* Revert socket protocols
Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need fix static analysis

.github/workflows/mutation.yml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
src/Handler/CompositeHandler.php Outdated Show resolved Hide resolved
src/Handler/CompositeHandler.php Outdated Show resolved Hide resolved
src/Handler/StreamHandler.php Outdated Show resolved Hide resolved
src/Handler/StreamHandler.php Outdated Show resolved Hide resolved
src/Handler/StreamHandler.php Outdated Show resolved Hide resolved
src/Handler/StreamHandler.php Outdated Show resolved Hide resolved
xepozz and others added 5 commits October 4, 2023 07:32
Co-authored-by: Sergei Predvoditelev <sergei@predvoditelev.ru>
Co-authored-by: Sergei Predvoditelev <sergei@predvoditelev.ru>
@vjik vjik merged commit 34b8c47 into master Oct 8, 2023
21 checks passed
@vjik vjik deleted the upd-handler branch October 8, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:code review The pull request needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants