search

RusticiSoftware / TinCanPHP

PHP library for the Experience API (Tin Can API)
http://rusticisoftware.github.io/TinCanPHP/
Apache License 2.0
87 stars 80 forks source link

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

Closed WillSkates closed 7 years ago

WillSkates commented 7 years ago

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.

pondermatic commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

I'll go ahead and close this one.