Skip to content

Conversation

@clue
Copy link
Contributor

@clue clue commented Jul 21, 2020

This minimal changeset updates react/http to the stable v1.0.0 released a few days ago: https://github.com/reactphp/http/releases/tag/v1.0.0

The tests are broken as of #503 which applied an incomplete update of this component. I've updated the code to resolve some minor BC breaks, but this doesn't seem to be covered by the existing unit tests afaict. It looks like this should be covered by some of the integration tests that are defined in the Travis CI configuration, I'm fairly confident this should verify this piece of code.

Let me know if there's anything else that needs to be done to get this merged 👍

@andig andig merged commit 459eaa8 into php-pm:master Jul 21, 2020
@clue clue deleted the reactphp-http branch July 21, 2020 09:40
@andig
Copy link
Contributor

andig commented Jul 21, 2020

Seems this would be ok with a minor release as the changes are internal to ppm?

@clue
Copy link
Contributor Author

clue commented Jul 21, 2020

Agreed, 👍 from my end :shipit:

This release might show some different performance characteristics mainly due to reactphp/http#371, so I think this warrants a v2.1.0 release 👍

@andig
Copy link
Contributor

andig commented Jul 21, 2020

What I actually wanted to suggest was a 2.0.5 as no APIs are changed for the typical use case as we rely on PSR for interfacing with the client application?

@clue
Copy link
Contributor Author

clue commented Jul 21, 2020

I agree that no APIs have been changed, so I'll leave this decision up to you ultimately 👍

I would argue that in the SemVer sense ("breaking.feature.bug") this could be considered a feature rather than a bug fix (together with #503). I would consider "stable LTS" a feature on its own, but this might just be my personal preference. Either way works for me, let's just get this shipped :shipit:

@klemenb
Copy link
Contributor

klemenb commented Jul 23, 2020

I've been checking up on this project lately as we are experimenting with PHP-PM at our company and it will probably replace our PHP-FPM deployment in the coming months.

I would just like to give an independent take on the 2.0.5 vs 2.1.0 debate as our use case sees a rare (but consistent) number of request bodies above 64K in size. This happens mostly when apps that were offline for a longer time send back some internal activity data. The number is way below 0.1% of all requests, but in this case this even exacerbates the issue, since we wouldn't pay that much attention to that and the problem would probably stay below our radar for some time.

In our case, if the change would be in 2.0.5 and I wouldn't pay close attention to the changes in a minor release, this change would break those requests, so a 2.1.0 release would probably be a better choice. I agree that 64K is enough for the general use case, but often there are just enough exceptions that there might be issues when it comes to introducing such a change.

Also, will it be possible to change these values via PHP-PM configuration so that we can adjust them for our use case?

@andig
Copy link
Contributor

andig commented Jul 24, 2020

Thanks for your feedback- released at 2.1.0.

@clue the last "unstable" component we have now is react/child-process ;)

@clue
Copy link
Contributor Author

clue commented Jul 24, 2020

Thanks for your feedback- released at 2.1.0.

Awesome!

@clue the last "unstable" component we have now is react/child-process ;)

We're working on changing this as soon as time permits! 💪 You know how to help speed this up ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants