Introduce DecoderManager

[peterpostmann]

Hi,

this is PR 3/3 as discussed in #12.

A DecoderManager is used to manage different decoders for different media types. The logic which decoder may be used is implemented in the Loaders and based on the output of the new determineMediaType function.

determineMediaType evaluates first the Content-Type header, then the file extension and defaults to null if no type can be determined. The DecoderManager has an option to register a defaultType which is used in case that the type is unknown or can not be determined. If no arguments are provided, the DecoderManager registers the Json-Decoder and makes it default for all types. The Yaml decoder is not register per default as discussed.

Open Points

determineMediaType

Right now the decoder fails if the content type is set, but no decoder is registered for this content type, except for the content type "application/octet-stream". In that case the file extension is used if present.

I think functions is not the right place for this logic, maybe it would be better to do it in the DecoderManger and make it pluggable. I'm not so happy about the huge amount of logic which is already there, because it get too complex, but I dont know a better solution at the moment.

Integrating the logic with the DecoderManger opens another option, namely to test if parsing is possible if now type can be determined from the headers or file extension. Then the DecoderManager should keep track about the Decoders in such a way, that no decoder is tested twice.

parseContentTypeHeader, parseHttpResponseHeader

Those functions may be extracted to their own library?

getDecoderManager, setDecoderManager (LoaderManager)

The LoaderManager has a getDecoderManager method which allows the DecoderManager to be access (e.g. to add a Decoder) easily.

$deref  = new Dereferencer();
$decoder = new YamlDecoder($deref->getLoaderManager()->getDecoderManager());
$deref->getLoaderManager()->getDecoderManager()->registerDecoder('yaml', $decoder);
$result = $deref->dereference('http://localhost:1234/albums.yaml');

Instead of passing the Decoder directly to the Loader, the DecoderManager is passed. The DecoderManager can not be replaced easily, since it would have to be replaced in all Loaders. This would requiere a new function in each Loader or a Proxy Object. Therefore the LoaderManger does not have a setDecoderManager. This feals a bit complex. Maybe there is a better way to manage this?

Looking forward for Feedback!

[matt-allan]

Hello,

Thanks for putting so much work into this. Sorry it has taken me so long to respond. This PR is quite large and introduces some major architectural changes.

I think functions is not the right place for this logic, maybe it would be better to do it in the DecoderManger and make it pluggable.

It seems like the strategy for picking the decoder is largely dependent on the loader. The web loaders would use Content-Type, the file loader would use the extension, the array loader might have a map of key => decoder, etc. Putting it in a single function means every loader has to use every strategy and makes it difficult to change.

I'm not sure if the key => value map of decoders needs to be shared between all the loaders. Right now you need to register keys for content types and extensions in the same map. It might make more sense for each loader to have it's own decoder manager that only contains keys for the strategy it is using.

parseContentTypeHeader, parseHttpResponseHeader
Those functions may be extracted to their own library?

Introducing logic for parsing response headers makes this library more difficult to maintain and is outside of the scope of what this library intends to solve. If we need this level of content negotiation we should definitely consider using a library that provides this.

The default content-types that are being registered are really context dependent and I don't know if we can provide sensible defaults for every use case. JSON Schema uses application/schema+json (but lots of servers use application/octet-stream), swagger is currently using application/vnd.oai.openapi and application/vnd.oai.openapi+json, regular JSON uses application/json or *+json, etc. Since JSON references can be used with any JSON or YAML document and YAML doesn't have an official content-type it's hard to provide defaults that will make everyone happy.

It seems like the web loader should probably be sending accept headers too, but that depends on the type of document being requested.

getDecoderManager, setDecoderManager (LoaderManager)

It seems like the loader manager should not have to know about the decoder manager; it doesn't ever use it directly. We are essentially passing a service locator to a service locator. It seems like it's only necessary because we are sharing the same decoder manager between all loaders.

Implementing this feature while maintaining backwards compatibility makes the feature more complex. It would probably be worth holding off on this until a new major version is tagged.

I wonder if there is a way to solve this problem for the 90% use case with less complexity? It seems like most users want to pick the decoder for HTTP using the Content-Type header and the decoder for files using the extension. Maybe providing a web loader and file loader that each accept an array of decoders is enough?

I'm going to close this for now. I opened an issue for further discussion of how this might be implemented.