search

php-http / utils

[DEPRECATED] HTTP Client Utilities
http://httplug.io
MIT License
2 stars 8 forks source link

Update to last change of httplug #6

Closed joelwurtz closed 8 years ago

joelwurtz commented 8 years ago

I update the library (tests are ok if the 2 pr are merged, since actually the httplug dependency in this package still has the options parameters in sendRequest) :

sagikazarmark commented 8 years ago

since actually the target repo for httplug still has the options parameters in sendRequest

Are you sure it is still the case?

joelwurtz commented 8 years ago

I'm pretty sure :)

utility_separation branch on httplug was based on a commit before this change (if you do a composer install here you will still have the $options parameter, unless #5 is merged)

sagikazarmark commented 8 years ago

Ah, the dependency thing. Sure.

sagikazarmark commented 8 years ago

Merged

joelwurtz commented 8 years ago

Yes that was not very clear sorry :/

sagikazarmark commented 8 years ago

@joelwurtz thanks for this PR. We really needed this and I have to admit, I did not have the patience to do this refactoring.

I commented on a few minor things, but no big deals. Can you check them please? Thank you in advance.

joelwurtz commented 8 years ago

This should be good, and rebase against master

sagikazarmark commented 8 years ago

I've spotted a few other issues which must be solved:

@joelwurtz May I ask you to check these out too?

sagikazarmark commented 8 years ago

This should be good, and rebase against master

Sorry, I am not sure I can follow. What do we have to rebase?

joelwurtz commented 8 years ago

Sorry, I am not sure I can follow. What do we have to rebase?

Just say my PR was rebased against master (since the merged in #5).

I've spotted a few other issues which must be solved:

Exceptions must also be cloned here Most of docblocks can be be removed and replaced by inheritdoc @joelwurtz May I ask you to check these out too?

No problem

sagikazarmark commented 8 years ago

Ah, I see. Great and thanks.

joelwurtz commented 8 years ago

Not sure how to clone the exceptions array as an exception is not clonable ?

sagikazarmark commented 8 years ago

It is not an array, but an SplObjectStorage. Although you made a point. Exceptions themselves are not cloneable.

Can you check out which is the default behaviour of cloning an SplObjectStorage: are the underlying objects cloned too?

joelwurtz commented 8 years ago

I add a test to valid the behavior (like adding an exception then a request which should keep the previous exception) it seems to work.

So i believe that underlying object are not cloned only the array but reference are still the same

sagikazarmark commented 8 years ago

Hm. Not sure if it's bad or good. But let's keep it that way then for now.

Pinging @dbu to let him know the situation. Do you think that this could cause problems in the future?

joelwurtz commented 8 years ago

Also i remove the BodyGenerator believing it was part of the Body classes, but now i'm not sure, what was the purpose of that interface ?

sagikazarmark commented 8 years ago

I intended to use the BodyGenerator name instead of Body as the idea was to use them to generate string body and headers which then can be injected into one of the HTTP methods instead of the Body object itself. Then we invalidated the idea, so Generator can go as well.

sagikazarmark commented 8 years ago

About BatchResultSpec: the initialized object should be checked if it is a BatchResult OBJECT, and a new test should be added to test if the current subject implements our BatchResult INTERFACE.

joelwurtz commented 8 years ago

About BatchResultSpec: the initialized object should be checked if it is a BatchResult OBJECT, and a new test should be added to test if the current subject implements our BatchResult INTERFACE.

Is the good behavior now ? (not used to phpsec :/)

sagikazarmark commented 8 years ago

Perfect. Is it ready for merging?

joelwurtz commented 8 years ago

Think so, do we wait for dbu input about cloning SplObjectStorage ?

sagikazarmark commented 8 years ago

Nope, that's just a note to him, also this is how I originally solved it, so even if it is wrong, we can stick to it in the update context.

Further thinking about this: technically BatchResult is immutable. The object itself, the storage objects and the request objects are immutable. Exceptions are mostly read-only.

sagikazarmark commented 8 years ago

Thank you @joelwurtz