mjaschen/collmex

Refactor Request::send and the ResponseFactory

Opened this issue · 2 comments

oliverklee commented

Currently, Request::send can return either a ZipResponse or a CsvResponse (depending on the MIME type of the HTTP response). Both Response classes share the same interface ResponseInterface. However, this interface is empty, and the caller still needs to know which response type to expect in order to properly use the response. This feels very dirty to me (and to static code analysis).

@mjaschen Do you know which types will create a ZipResponse?

Off the top of my head, I can think of the following approaches (most of will include mark the old Request::send method as @deprecated, replacing it with something cleaner and removing it for version 2.0.0):

  1. drop ZipResponse (and ParserInterface altogether)
  2. create a new send method (with a different name) that will always return an array of CsvResponses - either with one element or potentially multiple elements (for ZIP responses)
  3. have two sendmethods: one for ZIP and another for CSV

@mjaschen What do you think?

oliverklee commented

@mjaschen I'll really appreciate you input on this before I start implementing.

mjaschen commented

ZipResponses are returned for several types (Quotations, Orders, Invoices, Deliveries).

From a library user's perspective I'd prefer an implementation described as item (2).

To clarify:

  • You propose extending the CsvResponse so that it optionally contains the corresponding PDF file (which we've to extract from the ZIP and match it to the proper CSV line)?
  • Multiple response records will be returned as an array from the new send() method?