[Console] application/command as text/xml/whatever decoupling #7454
Conversation
| /** | ||
| * Returns given object's representation. | ||
| * | ||
| * @param mixed $object |
There was a problem hiding this comment.
it should be @param object if $object can only be an object. And if it can be something else (a scalar or an array), the argument should be renamed)
|
This PR is massive. It should be broken down into smaller commits that can be reviewed more easily. If possible perhaps even into separate smaller PRs that can be applied atomically without breaking stuff. |
|
@igorw I just unsquashed my commits for you. |
| { | ||
| $this | ||
| ->configure($options) | ||
| ->add(new Text\ApplicationTextDescriptor()) |
There was a problem hiding this comment.
Wouldn't it be a lot simpler if the application text descriptor itself were responsible for rendering its elements? I think it would simplify the design and lead to less implicit dependency on the order of the descriptors.
There was a problem hiding this comment.
I'm not sure to understand, this class just acts as a "descriptor selector".
|
I'm not sure if introducing the |
|
@igorw do you think the markdown formatter is overkill? |
|
Possibly. I guess re-implementing pandoc inside of symfony seems like a bit much. |
| * | ||
| * @return string A string representing the InputDefinition | ||
| */ | ||
| public function asText() |
There was a problem hiding this comment.
Removing the public method is a BC break. you should provide a BC layer
There was a problem hiding this comment.
I guess asXml() methods should be present too and must be tagged as @deprecated.
|
@igorw I think your right, I'll simplify the markdown descriptor. |
|
|
||
| // reads native definition | ||
| $method = new \ReflectionMethod($this->command, 'getNativeDefinition'); | ||
| $method->setAccessible(true); |
There was a problem hiding this comment.
Relying on reflection to get the definition looks like a hack
|
I don't like the hard coupling of descriptors, where each descriptor creates new instances of other descriptors. I leads to several issues:
You should retrieve the needed descriptor from the selector IMO |
|
@fabpot absolutely. Should I do the same with |
|
Let's do that in the same PR (and indeed the router debug command should also be updated). |
|
Can you also removed the |
| { | ||
| /** | ||
| * @param InputArgument $argument | ||
| * |
There was a problem hiding this comment.
Can you remove the blank lines between @param lines?
…fsimon) This PR was squashed before being merged into the master branch (closes #7887). Discussion ---------- [FrameworkBundle] adds routing/container descriptors The goal of this PR is to add descriptors (as in #7454) for routing and container. This will permit add a `--format` option to `router:debug` and `container:debug` commands (with `txt`, `json`, `xml` and `md` formats). Commits ------- 22f9bc8 [FrameworkBundle] adds routing/container descriptors
This PR removes description generation from
Command,ApplicationandInputDefinitionclasses and delegate it to specialized descriptor classes, making it dead simple to add new output formats.Maybe this could include other commands, like
router:debugorcontainer:debug(see #5740)?DescriptorProviderwhich usesDescriptorInterfaceobjects to describe things.txtdescriptors.xmldescriptors.jsondescriptors.mddescriptors.