Skip to content
This repository has been archived by the owner on Jan 17, 2022. It is now read-only.

feat(http_server): add max_request and max_request_grace configuration options #220

Merged
merged 1 commit into from May 20, 2020

Conversation

luca-nardelli
Copy link
Contributor

Hi @k911,

First of all, many thanks for the amazing bundle!
I needed to set the max_request and max_request_grace options from the bundle configuration so I thought of adding them.

I couldn't find any tests related to the configuration parsing and passing to Swoole, should I create one? If so, should I just test the HttpServerConfiguration class or should I also test parsing from Symfony's side?

Let me know, thanks!

@k911
Copy link
Owner

k911 commented May 19, 2020

@luca-nardelli Thanks, it looks ok to me. Just wondering if we shouldn't prefix those options with: worker_, so it'd be more clear when using CLI

You can also add them to the https://github.com/k911/swoole-bundle/blob/develop/docs/configuration-reference.md

Currently, there are no unit tests for configuration component and since there is a major refactor going on on feature branch - so I'm fine with changes

@codecov
Copy link

codecov bot commented May 19, 2020

Codecov Report

Merging #220 into develop will decrease coverage by 0.09%.
The diff coverage is 84.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #220      +/-   ##
=============================================
- Coverage      87.19%   87.09%   -0.10%     
- Complexity       604      612       +8     
=============================================
  Files             84       84              
  Lines           1858     1883      +25     
=============================================
+ Hits            1620     1640      +20     
- Misses           238      243       +5     
Impacted Files Coverage Δ Complexity Δ
src/Server/HttpServerConfiguration.php 89.55% <75.00%> (-2.83%) 63.00 <6.00> (+8.00) ⬇️
...fony/Bundle/Command/AbstractServerStartCommand.php 85.38% <100.00%> (+0.22%) 30.00 <0.00> (ø)
...mfony/Bundle/DependencyInjection/Configuration.php 97.87% <100.00%> (+0.08%) 7.00 <0.00> (ø)

@luca-nardelli
Copy link
Contributor Author

Hey, the prefix seems like a good idea to me as it makes the options clearer to understand.

I should have updated the PR with the prefix and with the documentation update as well

@luca-nardelli
Copy link
Contributor Author

Edit: I changed the node definition for worker_max_request_grace to scalarNode so that we can support null as well (with integerNode this was not supported)

@luca-nardelli
Copy link
Contributor Author

Forgot to ran composer fix, I updated the PR

@codeclimate
Copy link

codeclimate bot commented May 20, 2020

Code Climate has analyzed commit 2585564 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

The test coverage on the diff in this pull request is 84.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 87.0% (0.0% change).

View more on Code Climate.

@k911
Copy link
Owner

k911 commented May 20, 2020

@luca-nardelli Thanks for this contribution, I'll release this soon

@k911 k911 merged commit 69fd435 into k911:develop May 20, 2020
Development automation moved this from In progress to Done May 20, 2020
k911 added a commit that referenced this pull request May 20, 2020
* fix(server): add worker_max_request and worker_max_request_grace configuration options (#220) ([69fd435](69fd435)), closes [#220](#220)
* build(deps-dev): bump phpstan/phpstan from 0.12.19 to 0.12.20 (#212) ([d2ca5ea](d2ca5ea)), closes [#212](#212)
* build(deps-dev): bump swoole/ide-helper from 4.4.17 to 4.5.0 (#197) ([021b84a](021b84a)), closes [#197](#197)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants