A few questions

[omares]

Hey, i am currently testing your lib and planning to write some unittests. To fully understand the feature set it would be great if you could explain your intentions behind these features:

• Why the location rewrite feature and whats the benefit?
• Why do you remove the transfer-encoding and content-encoding headers from the response?

Thank you!

[omares]

I moved the questioned code parts into own classes (filters), so its configurable/enableable by the user now ;) rebuy-de/php-proxy#1

To use that new feature you would have to accept my PR #11 which includes a few BC-Breaks

[jenssegers]

The location rewrite is to prevent the user to be redirected to a different url immediately. Somethings it makes the user follow redirects in a loop, in other situations it can be helpful.

The transfer-encoding and content-encoding are removed so that the own webserver can set the correct headers if it uses a different encoding method.

[jenssegers]

I think it would be interesting to create some sort of Proxy\Transport interface/abstract class that handles the conversion of the Symfony request object to the actual Guzzle objects. For Guzzle there would then be a Proxy\GuzzleTransport class.

[omares]

The location rewrite is to prevent the user to be redirected to a different url immediately. Somethings it makes the user follow redirects in a loop, in other situations it can be helpful.

Ah i see, wouldnt it be better then to remove the location header and add is as a X-PHPProxy-Location Header?

The transfer-encoding and content-encoding are removed so that the own webserver can set the correct headers if it uses a different encoding method.

Okay, works for me.

I think it would be interesting to create some sort of Proxy\Transport interface/abstract class that handles the conversion of the Symfony request object to the actual Guzzle objects. For Guzzle there would then be a Proxy\GuzzleTransport class.

Basically thats exactly what i created with my converters in the referenced rebuy-de/php-proxy#1 PR

Btw. under which license is phpproxy published?

[jenssegers]

I quickly released this version because I wanted to integrate Symfony request and response objects. I will be making changes as soon as I can find some spare time.

All my packages are MIT.

[omares]

Hehe, how big are the chances you would merge the #11 PR? :)

[jenssegers]

Your PR has some great additions, but there are some things I would change though. Instead of the RequestConverterInterface, I would use "transporters" (or whatever) that receive a Symfony Request, do the actual http request and return a Symfony Response. I really like your filters. And I don't know about the namespace, wouldn't Proxy\Factory look nicer than Phpproxy\Factory?

[omares]

I would use "transporters" or whatever that receive a Symfony Request and return a Symfony Response.

Is the Converters vs Transportes just a naming issue? Or does symfony have a transporter module?

And I don't know about the namespace, wouldn't Proxy\Factory look nicer than Phpproxy\Factory?

Sure i am totally fine and agree with that. The namespace is based on the repository name ;). Or we could call it Phproxy :P

[jenssegers]

Transporters, clients, ... I don't know what the best name would be. It would work like this: when you construct a Proxy object, you inject a Transporter/Client instance using the constructor. The Proxy object is requested to forward a Symfony request to a certain url and after doing some filters, it passed the Request to the transporter/client which will return a Symfony Response. This way, the transporter will take care of converting the Request to something that it can use.

This way you would be able to do stuff like this:

$proxy = new Proxy(new GuzzleTansporter);

Or a:

$proxy = new Proxy(new DummyTransporter);

Or a:

$proxy = new Proxy(new CurlTransporter);

[omares]

Great idea! I'll try to implement that :)

[jenssegers]

And the included Factory can just create a Proxy instance with a default GuzzleClient/GuzzleTransporter.

[omares]

Here we go, the PR now includes Adapters (aka Transports) and the updated namespace. On second thought one could simply change the Adapter that guzzle uses, but the change makes the code more readable.

[omares]

Btw, i will close this issue now. We should continue our discussion in the #11 PR. I am keen to read your feedback.