Move Body, HttpError exceptions, BatchRequest, HttpMethods to utility

[sagikazarmark]

Moves lots of stuff to the utility package as discussed in #56.

Implementations remaining in this package:

• Exceptions
• BatchResult

One thing in my mind: while the BatchResult will likely have only one implementation, we should consider moving it to the utility package and having an interface in the contract.

Reasoning:

• BatchResult itself requires two SPL exception extensions in the contract, which are otherwise not required. They could also be moved to the utility package
• BatchResult is implemented as a final class, which is fine, because I can hardly imagine a case where extension would make sense. However having it as a concrete implementation forces everyone to use that class, instead of simply requiring an interface. While it might be an ugly hack, but some processing functionality might be implemented in the BatchResult class.
• BatchResult is implementation and we should have as little implementation in the contract as possible.

What do you think guys?

[dbu]

good cleanup! can you link the PR that adds the parts that we keep to the utils repository?

why exactly do you drop the body implementations? is there a Body factory? otherwise a client has a problem as it would need to new Body but does not know what implementation is available. maybe we need to keep those in here.

• BatchResult itself requires two SPL exception extensions in the contract, which are otherwise not required. They could also be moved to the utility package

i think the exceptions can not be moved because they must be part of the contract. users must be able to catch those specific exceptions.

What do you think guys?

but i agree with the other two points, lets have an interface for BatchResult. the main use case i would see is when the underlying http client already provides a similar class and we want a wrapper class instead of an implementation.

[sagikazarmark]

I haven't created the util repository yet. Not until we decide what exactly goes there from this repo.

Actualy there is a problem with moving BatchResult to the util repo: BatchException currently creates a new one if none is passed when using the getter. It's a hack to make sure a result is always available. How should we workaround that? Throw an exception if the result is not set?

[joelwurtz]

We can maybe force a BatchResult into the constructor of the Exception and remove the creation in the getResult ? (The other choice would be to allow null for the getResult method)

EDIT : Throw an exception in a exception is a no go for me

[sagikazarmark]

I already thought about that. The problem is that BatchResult is immutable which would cause a lot of confusion. It is injected but the incorrect version is in the exception.

[joelwurtz]

Didn't see the immutable part, so this is incorrect https://github.com/php-http/adapter/blob/master/src/Util/BatchRequest.php

[sagikazarmark]

Yup, I already noticed that during the movement of classes from the main repo. We can correct it in the util repo once it gets clear what exactly will be in it.

[joelwurtz]

We can otherwise make BatchException implement this interface, so BatchException is a BatchResult ?

[sagikazarmark]

BatchResult should be immutable, exceptions cannot be made immutable: they cannot be cloned.

[sagikazarmark]

@dbu If you have some time, please push commits to this PR.

[dbu]

pushed some phpdoc cleanup. the only open thing from my side now is https://github.com/php-http/adapter/pull/59#discussion_r40521150

[sagikazarmark]

why exactly do you drop the body implementations?

I plan to add body generators in the utility package. This also means, that a Body object cannot be passed to the MethodsClient anymore. But the contract is more elegant this way IMO.

i think the exceptions can not be moved because they must be part of the contract. users must be able to catch those specific exceptions.

I think we can rely on SPL exceptions in this case. The common exception makes sense when you don't care about what really happend, you want to catch all DomainExceptions. But SPL exceptions are used in cases that you called "program errors". If we need an exception for something, which indicates something else then a user error, then we should create our custom exception for that.

[sagikazarmark]

PSR-7 also relies on SPL Exceptions.

[dbu]

what about the client and server exceptions for 4xx and 5xx ? i remember we discussed back and forth about throwing exceptions versus returning a response with an error status in it. what did we decide in the end?

[sagikazarmark]

We decided to move this functionality to a Plugin.

[sagikazarmark]

Is there anything we need to do before merging this one?

[dbu]

yay!