parse-community / parse-php-sdk

The PHP SDK for Parse Platform
https://parseplatform.org/
Other
811 stars 346 forks source link

Integration for HHVM Support #342

Closed montymxb closed 7 years ago

montymxb commented 7 years ago

This is an attempt to integrate official HHVM support for the parse-php-sdk. Only a couple test changes were needed to be made up front, but the underlying stream client seems to misbehave.

Given that full compatibility will be required for this to move along I'll be tweaking things here to see if I can get the full test suite (with the stream tests) to play nice with hhvm.

montymxb commented 7 years ago

This almost works, however support for the stream client is not currently working with hhvm, specifically due to it sending duplicate headers. The curl client is fine however, and all the other tests indicate it is stable.

I've opened up #4030 in parse-server to entertain the idea of handling duplicate headers for api keys, tokens, etc. The alternative to that is to possibly wait for a new release of hhvm, or to try submitting a patch to address the issue there.

codecov[bot] commented 7 years ago

Codecov Report

Merging #342 into master will decrease coverage by 0.06%. The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
- Coverage   98.67%   98.61%   -0.07%     
==========================================
  Files          35       35              
  Lines        3164     3171       +7     
==========================================
+ Hits         3122     3127       +5     
- Misses         42       44       +2
Impacted Files Coverage Δ
src/Parse/ParsePolygon.php 100% <100%> (ø) :arrow_up:
src/Parse/HttpClients/ParseStreamHttpClient.php 97.95% <75%> (-2.05%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cbb6301...b470f2d. Read the comment docs.

montymxb commented 7 years ago

For historical purposes I'm just going to mention what was done here.

Turns out any header you pass to header when creating a new stream context will be duplicated in all observed cases (observed on hhvm 3.20). In order to get past this and add HHVM support I was dug into the http-stream-wrapper and found a nifty looking bit of c++ :

if (opts.exists(s_user_agent) && !headers.exists(s_User_Agent)) {
    headers.set(s_User_Agent, opts[s_user_agent]);
}

From this it appeared that the User-Agent field, passed in the options to create a stream context, was not sanitized. Although this is a separate concern running a git blame shows this has been around since 2013 too.

Taking advantage of this I was able to append the custom headers after the User-Agent string.

$this->options['http']['user_agent'] = "parse-php-sdk\r\n".$this->buildRequestHeaders();

Turns out this works pretty well, and it's also something that can be done on php5, and probably up to 7 (but I wouldn't recommend it!). In this case it's strictly implemented to handle hhvm.

montymxb commented 7 years ago

Cool! just wanted to have another set of eyes check this over since it isn't exactly a standard approach. And yeah with HHVM this could open up a whole new realm of possibilities 👍

flovilmart commented 7 years ago

Is it a heavily requested feature?

montymxb commented 7 years ago

No I actually haven't heard anything about it in particular, just figured it would be good to add though. Especially considering how much more efficient it can be.

Plus travis offers that ever so enticing hhvm option for testing, wanted to see if we could cover all those offered bases.