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

Adding PHP 7.1.x to the build environment list. #83

Closed
wants to merge 9 commits into from

Conversation

WillSkates
Copy link

No description provided.

@WillSkates
Copy link
Author

This seems fine to merge.

I have moved hhvm to allowed failures. The fopen() call in RemoteLRS is throwing an "Array to string conversion" when the only dynamic inputs are a string and a resource.

I'm guessing that this is an upstream bug at this point.

@WillSkates WillSkates mentioned this pull request Nov 4, 2017
@pondermatic
Copy link

@WillSkates, I found that HHVM sends an 'Array to string conversion' notice when the stream context http header is an array. An easy fix is to convert it convert it to a string with HTTP CRLF line breaks. Check out pull request #96. Sorry I ended up duplicating most of your pull request. I didn't notice it until it was too late.

@pondermatic
Copy link

The HHVM 'Array to string conversion' issue was reported 2017-02-17.
file_get_contents() raises notice when using http header option #7684
It seems that the notice was added between HHVM 3.12.2 and 3.18.0.

@WillSkates
Copy link
Author

WillSkates commented Nov 15, 2017

Hi @pondermatic I thought that might be the case, it's what I meant by "upstream bug" but that isn't too clear.

I wouldn't change the code to adapt to this case, instead add HHVM to the list of allowed failures and wait for Travis to provide us with a more up to date HHVM. We can't confirm that this fix works with a later version of HHVM either until it runs successfully in Travis with that later version (we may be able to specify the hhvm version later than 3.18 but it isn't listed here).

Other than that I much prefer your version of these fixes. The only other thing I think @brianjmiller may ask is that you don't merge your other branches with the PHPUnit fixes one. Of coarse you won't get the green tick but a note saying "#96 should be merged first" should be sufficient. It just means that the PR only contains the set of changes for the subject of that PR.

@pondermatic
Copy link

Hi @WillSkates, Thank you for the feedback.

I fixed several issues related to stream context http headers in HHVM and submitted a PR. If I revert the change made in TinCanPHP that makes it work with HHVM 3.15.0+, should we add a note in README about compatibility?

I will revert the merges from the PHPUnit PR. This will keep each PR focused on one category of issues at a time.

@WillSkates
Copy link
Author

@pondermatic well done on the PR for HHVM! You're absolutely right we should add a note in the README. I'd also keep HHVM in the allowed failures list until your PR is merged. That should be enough to make it clear as to what is going on.

@WillSkates
Copy link
Author

I'll go ahead and close this one.

@WillSkates WillSkates closed this Nov 20, 2017
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.

2 participants