Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getConfig() is being called early in controller if getParameters() is called. #5

Closed
jcloutz opened this issue Jun 2, 2015 · 3 comments
Assignees
Milestone

Comments

@jcloutz
Copy link

jcloutz commented Jun 2, 2015

I just found some strange behaviour in the execution order of JsonApiTrait.

Assuming the following methods added to the base controller:

// Controller.php
protected function setSchema($schemas = [])
{
    $this->schema = $schemas;
}

protected function getConfig()
{
    $config = $this->getIntegration()->getConfig();
    $this->schema === null ?: $config[C::SCHEMAS] = array_merge($config[C::SCHEMAS], $this->schema);

    return $config;
}
// AuthorsController.php
public function show(Request $request, $id, $type)
{
    // this causes getConfig to be called early
    $params = $this->getParameters();

    $this->setSchema([
        Post::class => function ($factory, $container) {
            $schema = new PostSchema($factory, $container);
            $schema->renderAsRelation = true;

            return $schema;
        },
    ]);

    return $this->getContentResponse(Author::findOrFail($id)->getPosts());
    }
}

If getParameters() is called inside a controller action before setSchema()
then getConfig() is also called before the schema can be injected into the
config array.

@neomerx
Copy link
Contributor

neomerx commented Jun 2, 2015

Fixed in develop branch. Will be merged to master with json-api next montly release

Additionally you can set encoder this way

public function show($idx)
{
    ...
    // trigger parse params and headers
    $this->checkParameters();

    // optional media type check 
    $mediaType = $this->getCodecMatcher()->getEncoderRegisteredMatchedType();
    if (MediaTypeInterface::JSON_API_MEDIA_TYPE === $mediaType->getMediaType()) {
        $this->getCodecMatcher()->setEncoder(function () {
            $encoder = ...; // e.g. Encoder::instance(...);
            return $encoder;
        });
    }

    return $this->getContentResponse(...);
}

tl;dr (history of the fix)

The root of the problem is that this trait is trying to be helpful (maybe too much 😄)

When getParameters is called it works this way

  • it needs parameters from URL and controller configuration to compare (no problem here)
  • it decodes media types of received data (Content-Type header) and requested (Accept header). Then it wants to check if those types are supported (that's the root of the problem). It takes all registered encoders and decoders, finds best matching media types, encoder and decoder. While doing this it instantiates encoder (in EncoderMatcher::matchEncoder) (that's likely unnecessary thing to do) which leads to getConfig call.

What are the possible solutions?

Firstly I feel coupling between parameter parsing and matching encoder/decoder has to be changed. It doesn't look beautiful to intercept it with overriding getConfig when a developer actually wants to say 'use this encoder' or 'use these encoder settings'.

I think getParameters should parse and check URL parameters and media types in headers however encoder and decoder should not be instantiated. A developer should have an option to modify encoder/decoder/their settings before getContentResponse is called. For example

  • get media types (requested/matched) and set new encoder/decoder (object or closure)
  • get media types (requested/matched) and set parameters that will be sent on closure call

I tend to think the first variant is more flexible. In pseudo-code it might be

    if ($this->getEncoderMediaType()->getType() === 'application/vnd.api+json') {
        $this->setEncoder(Encoder::instance([
            Author::class  => AuthorSchema::class,
           ...
        ]);
    }

If such a snippet is not added to Controller handler (e.g. show) it will use default encoder/decoder.

What do you think?

@jcloutz
Copy link
Author

jcloutz commented Jun 3, 2015

This works for me, and it makes more sense to be able to inject your own encoder vs just overriding an element in the config array.

@neomerx
Copy link
Contributor

neomerx commented Jun 3, 2015

Nice to hear it 😄

neomerx added a commit that referenced this issue Jun 3, 2015
neomerx added a commit that referenced this issue Jun 3, 2015
@neomerx neomerx closed this as completed Jun 10, 2015
@neomerx neomerx removed the fixed label Jun 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants