php-http / message

HTTP Message related tools
http://php-http.org
MIT License
1.3k stars 42 forks source link

Add message, stream and URI factories for Slim Framework #53

Closed tuupola closed 8 years ago

tuupola commented 8 years ago
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Related tickets
Documentation https://github.com/php-http/documentation/pull/159
License MIT

What's in this PR?

This PR adds message, stream and URI factories for Slim Framework

Why?

Slim is reasonably popular PSR-7 based framework.

Example Usage

use Http\Client\Curl\Client;
use Http\Message\MessageFactory\SlimMessageFactory;
use Http\Message\StreamFactory\SlimStreamFactory;

$client = new Client(new SlimMessageFactory(), new SlimStreamFactory());

Checklist

tuupola commented 8 years ago

Build fails on PHP 5.4 because Slim requires PHP >=5.5.0.

sagikazarmark commented 8 years ago

@tuupola thank you for the PR. Can you try adding https://github.com/akeneo/PhpSpecSkipExampleExtension and skip the tests on PHP 5.4?

tuupola commented 8 years ago

That might not be enough since also composer install fails. I see what I can do with the Travis build matrix.

sagikazarmark commented 8 years ago

@php-http/owners how about dropping PHP 5.4 support of this package too?

Nyholm commented 8 years ago

Dropping 5.4 is a no-brainer in my opinion. My general approach is that you should keep old PHP versions if there is no reson for bumping then. I believe @tuupola has an excellent reson here.

We had a discussion on twitter about supporting 5.4 because legacy project would not be able to update to HTTPlug without dropping support for 5.4. (Because of php-http/discovery).

xabbuh commented 8 years ago

As this library is only a dependency during development I would not use it as an argument to bump the minimum version requirements. Instead I would remove the dev requirement and install the package on Travis CI only for PHP >= 5.5 jobs and skip the example otherwise as @sagikazarmark suggested.

sagikazarmark commented 8 years ago

@xabbuh the problem is that this way development is much harder. I would do the other way around: remove the library in CI on PHP 5.4. That way you can't develop it if you have 5.4, but the lib remains compatible with it.

Nyholm commented 8 years ago

Sure, Im fine with that.

xabbuh commented 8 years ago

@sagikazarmark Sounds good too.

tuupola commented 8 years ago

I already have it the way @xabbuh explained. However comment by @sagikazarmark makes sense. I will do it over and commit soonish.

tuupola commented 8 years ago

Seems there is problem with streams with hhvm. Will install hhvm locally to figure it out.

tuupola commented 8 years ago

I would also like to add something to message factory tests. Namely I would like to test that factories set the message headers and body correctly. Should this be a separate PR?

Nyholm commented 8 years ago

Great job. Thank you!

sagikazarmark commented 8 years ago

@Nyholm there were some TODOs which indeed would have been nice to have here.

Nyholm commented 8 years ago

Sorry, I must have missed that. I saw that you approved it so I made a final review myself.

What were the TODOs?

sagikazarmark commented 8 years ago

See the TODO section: puli.json, possibly rename to Slim3

Nyholm commented 8 years ago

Sorry for not bringing those up to discussion before merging.

Yes, some entries should be added to Puli.json. The other two are fine IMHO

tuupola commented 8 years ago

I am not familiar with Puli. How should I add the entries to puli.json? I quickly read through puli docs but did not become any smarter.