Update to last change of httplug

[joelwurtz]

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) :

• Remove Body implementation according to Remove Body or include in HttpMethodsClient #3
• Remove options from sendRequest
• Readapt BatchResult with changes from BatchResult / BatchException

[sagikazarmark]

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

Are you sure it is still the case?

[joelwurtz]

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]

Ah, the dependency thing. Sure.

[sagikazarmark]

Merged

[joelwurtz]

Yes that was not very clear sorry :/

[sagikazarmark]

Can we test the BatchResult implementing the BatchResult interface here?

[sagikazarmark]

@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]

This should be good, and rebase against master

[sagikazarmark]

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?

[sagikazarmark]

This should be good, and rebase against master

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

[joelwurtz]

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]

Ah, I see. Great and thanks.

[joelwurtz]

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

[sagikazarmark]

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]

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]

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]

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]

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]

Slash is not required in case of strings: they are imported fromthe root namespace anyway.

[sagikazarmark]

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]

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]

Perfect. Is it ready for merging?

[joelwurtz]

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

[sagikazarmark]

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]

Thank you @joelwurtz