-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
PHP 8 support #45
PHP 8 support #45
Conversation
Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
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 so far.
You might want to have a look on the failing unit tests.
Some methods got dropped in phpunit 9 so we have to find a better way to verify what've happened.
@boesing thanks. Yep, I'm having a look at the breaking tests on Travis at the moment. There are deprecations in PHP-CodeSniffer too, which is brought in by |
…brary Signed-off-by: Drew Robinson <[email protected]>
I would prefer not upgrading codestyle in here to have a clear overview on what've changed or PHP 8.0. If there are issues with codesniffer, we can bump dev-dependency for that package to ensure it always works with PHP 8.0. |
Signed-off-by: Drew Robinson <[email protected]>
…iffer library" This reverts commit d1d3b03. Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
@ocean If I can help you, just ping me 👍 |
Sorry I haven't had a chance to get back to this until now @boesing I would happy with some help. The test issue I've found most annoying is the lack of ability to access protected properties e.g. here https://travis-ci.com/github/laminas/laminas-http/jobs/400060420#L555 |
No problem, we all have plenty of stuff to do 💪 So, you could do sth. like this: $valueFromProperty = $this->extractPropertyValue($objectYouWantToExtractThePropertyFrom, 'propertyName');
self::assertEquals($expectedValue, $valueFromProperty); Within the |
Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
@boesing GitHub still says there's a review to do on this PR, but I can't see one. Also it seems the TravisCI jobs for this repo have stopped running 😞 I ran tests on my local machine with PHP 7.4 and they seemed ok, but am keen to see what Travis does with PHP latest. |
@ocean Thats fine, we have to review manually with each supported PHP version as we ran out of build hours on travis. |
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.
https://travis-ci.com/github/ocean/laminas-http/jobs/452211509#L709
Hm, thats a tough one as I am not that deep into that stuff and TBH I don't have time to dig into that.
Maybe @domoran could provide some insights here as he added this test 4 years ago?
Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
Signed-off-by: Drew Robinson <[email protected]>
@boesing I updated the curl chunked response test to test against "less than 25 milliseconds completion" instead of 20 milliseconds. This has made the test pass, meaning we can move on from this hopefully. |
@ocean I tried in my local dev with php 8 and it working ok 👍 |
Please note, that the 20, 25 in this test do not mean "ms" but it defines the factor on how long it takes to parse the chunked response, compared to the baseline [2000 chunks a 1 byte] vs [2000 chunks a 1 byte + 1 chunk 10 MB]. Its kind of hard to make a prediction on how long this is supposed to take - in the original implementation due to a very bad string handling the 10 MB chunk was copied thousands of times, making the packet take multiple minutes to parse. Since a test was required to perform the change on the zend http library I made this test to reproduce the problem and see that it was fixed - however the test has only the purpose to make sure the specific problem of the old implementation is gone. So there is nothing wrong with increasing the asserted ratio as long as it is ensured that the absolute times of parsing such a response are still reasonable (maybe sub 500ms?). Please also note, that the response should have never been loaded into memory anyways, because with large responses this will break any memory limit anyway. But that change was too big to perform at that time. So I would suggest switching to another HTTP library anyway, that does not suffer from this issue. |
Co-authored-by: Frank Brückner <[email protected]> Signed-off-by: Drew Robinson <[email protected]>
@domoran Thanks very much for the clarification. I see, so it is more like "this test should take less than 20 times as long as parsing the initial baseline response". @samsonasik Thanks for your double-checking input too. The test was passing on my local as well, but failing on Travis (but only by one or two increments), so I guess it's mostly caused by the limited speed of the Travis CI servers. @froschdesign Thanks, updated that schema version string now, ready for review 👍 |
@boesing it is already next week, I will merge it right now ;) |
Oh, the milestone is 2.12, automatic release is not working |
trying create milestone 2.14.0 and re-close the milestone |
2.14.0 released https://github.com/laminas/laminas-http/releases/tag/2.14.0 🎉 |
May I ask why this was released without a PHP 8 compatible version of laminas/laminas-validator? |
@SamuelNitsche one thing at a time - laminas/laminas-validator#75 just needs reviews, if you got time, and then it can certainly be released IMO |
Got it, thanks @Ocramius! |
Thanks @samsonasik 😄 |
Updating to add PHP 8.0 support for #44
First draft of PR for TravisCI testing.