Add support for nested types. Fixes #55 #56

Closed
wants to merge 2 commits into
from

Projects

None yet

7 participants

@willdurand
Collaborator

Result:

What do you think?

@stof stof commented on the diff Jul 24, 2012
Parser/FormTypeParser.php
$form = $this->formFactory->create($type);
+ return $this->parseForm($form);
@stof
stof Jul 24, 2012

what if the root name is not empty ? you would need to pass a prefix here in such case (but it may require an additional property in the annotation to pass it)

@mtotheikle
mtotheikle Feb 8, 2013

I think the above is required as well because the controller can create a form with a root name of anything and the documentation must have the root name when using the sandbox in order to successfully submit the form.

@stof stof commented on the diff Jul 24, 2012
Parser/FormTypeParser.php
@@ -67,6 +78,17 @@ public function parse($type)
}
}
+ if ('' === $bestType) {
+ if ($type = $config->getType()) {
+ if ($type = $type->getInnerType()) {
@stof
stof Jul 24, 2012

what about using a different name to be more readable ? and what about using && instead of nesting 3 if without else ?

@mvrhov
mvrhov Aug 1, 2012

Also you are using an assignment operator and not a comparison one.

@willdurand
willdurand Aug 1, 2012 collaborator

the assignment operator is not a mistake...

@evillemez

Aside from the changes in the parser does anything else need to be modified to support nested input? I was going to allow nested input in the JmsMetadataParser as well.

@evillemez

So, after looking into this, I'm not sure what the best way to deal with nested parameters would be. The formatters currently expect to get an array for parameters from the ApiDocExtractor that looks like this:


array(
    'propertyOne' => array(
        'dataType' => 'string',
        'required' => false,
        'description' => 'A nice description',
        'readonly' => false
    ),
    'propertyTwo' => array(
        'dataType' => 'string',
        'required' => false,
        'description' => 'A nice description',
        'readonly' => false
    ),
)

If we just nest arrays we could end up with conflicts in the keys... what about having the doc extractor return objects instead of arrays, so the formatters could easily tell what is what? This theoretically could effect filters as well, but not requirements.

@evillemez

I suppose we could just have a children key or something, then continue nesting. Not sure how I feel about it.

array(
    'propertyOne' => array(
        'dataType' => 'string',
        'required' => false,
        'description' => 'A nice description',
        'readonly' => false
    ),
    'propertyTwo' => array(
        'dataType' => 'Domain\Object',
        'required' => false,
        'description' => 'A nice description',
        'readonly' => false,
        'children' => array(
            'propertyOne' => array(
                'dataType' => 'string',
                'required' => false,
                'description' => 'A nice description',
                'readonly' => false,
            ),
            'propertyTwo' => array(
                'dataType' => 'integer',
                'required' => false,
                'description' => 'A nice description',
                'readonly' => false,
            )
        )
    ),
)

Thoughts?

@beberlei

+1 - i need this :)

@evillemez

The more I think about it, I think my last suggestion is actually a good one. It wouldn't require api changes in anything, just extra documentation to note that there could be nested children properties. And the only thing that needs to change is some minor refactoring in the current formatters to handle the extra data.

@willdurand @stof What do you think?

@beberlei

hm why is this nesting even necessary? from a HTTP perspective the parameter is "propertyTwo[propertyOne]" as a string, nothing nested.

@evillemez

Maybe, but there's multiple ways to format the documentation. JSON schema for example - it would be a pain to have to parse all of the properties to figure out when you've encountered a new object to document. This way it's natural, it can still be compressed down easily if that's how you want to do it.

I would imagine the markdown output would probably document it the way you described. But for adding Swagger json support that wouldn't work very well.

EDIT: Didn't really answer the question. It's necessary to account for nesting because the formatters are receiving the data directly from code - maybe from a Form Type, maybe JMS Metadata, maybe some other as-yet-to-be-implemented mechanism. The models that get processed are often nested, so somehow we have to return that data to the formatters to present those relationships clearly. The formatters could compress things down, but if that's done in the parsing stage, we just lose data.

@willdurand
Collaborator

Agreed. children is fine to me

@Seldaek
Member
Seldaek commented Nov 7, 2012

This can be closed since #76 is merged or did I miss something?

@willdurand
Collaborator

Well, I didnt close it because of: #76 (comment)

@Seldaek
Member
Seldaek commented Nov 7, 2012

Ah ok I missed that.

@evillemez

Yeah, also, I have a branch where I'm working on standardizing the "dataType" param... I was going to include this as part of that. The reason for it being that in order to support Swagger there would have to be a set list of valid "dataTypes" anyway. Anyhow... I have a branch where I was working on it, but I got pulled off working on something else. I'm still going to come back to it, but I don't know when I can.

In the interim, though, the only thing that the FormTypeParser needs to do in order to support nested types is return a children array along with the parameters. Everything else should just work because of the other work I did with the JMS stuff. This should be really easy to implement as a separate PR, but I'm not familiar enough with the Form component to do it quickly.

Sorry for holding things up... :/

@willdurand willdurand closed this Sep 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment