Document interface method #3959
Conversation
greg0ire
force-pushed
the
fix_field_description_doc
branch
from
c695c43
to
ace5c84
Compare
| @@ -22,8 +22,11 @@ | |||
| interface BuilderInterface | |||
| { | |||
| /** | |||
| * @param AdminInterface $admin | |||
| * @param FieldDescriptionInterface $fieldDescription | |||
| * Adds missing information to the given field description from the model manager metadata, | |||
There was a problem hiding this comment.
Don't think the information is actually missing.
Isn't it more that the builder adds additional information, mainly for building the view ?
Are there cases where the FieldDescription is constructed outside ModelManager::getNewFieldDescriptionInstance() ? I couldn't find any.
Or is it that the metadata can change between when the FieldDescription is constructed and the time it is passed to this method ?
There was a problem hiding this comment.
Isn't it more that the builder adds additional information, mainly for building the view ?
Not sure the field description can be properly used in the view without this method being executed. Regarding your other questions… help @rande !
There was a problem hiding this comment.
@greg0ire Yep I think you're right.
What trips me up is that ModelManager::getNewFieldDescriptionInstance() populates the FieldDescription with metadata, then BuilderInterface::fixFieldDescription() populates it again...
There was a problem hiding this comment.
Ok I think I get it now...
FormContractor implements BuilderInterface but it's not a real builder like the other builders. It is more like a middle-man between the FormMapper and the "real" Symfony FormBuilder, so the FormMapper needs to call methods on it.
I think this method belongs in fact to the FormContractorInterface, because it can be private to the other "normal" builders as no one calls it from the outside.
Isn't it ?
There was a problem hiding this comment.
a middle-man between the FormMapper and the "real" Symfony FormBuilder
Hence the word "contractor", maybe ?
I think this method belongs in fact to the FormContractorInterface, because it can be private to the other "normal" builders as no one calls it from the outside.
Should we make it private then? If yes, it should be removed from the interface, shouldn't it ?
There was a problem hiding this comment.
As per the rest of the answer, I think I need to read some code before I can understand it, I'll try to do that soon.
There was a problem hiding this comment.
what a "real" builder is
The DatagridBuilder, ListBuilder & ShowBuilder concrete implementations, i.e SonataDoctrineORMAdminBundle\Builder\ListBuilder et al.
In short the fixFieldDescription method is needed only on the FormContractorInterface.
There was a problem hiding this comment.
I'm looking at the FormContractorInterface right now and getFormBuilder is implemented more or less the same way every time. getDefaultOptions has some differences.
- https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/master/Builder/FormContractor.php
- https://github.com/sonata-project/SonataDoctrinePhpcrAdminBundle/blob/master/Builder/FormContractor.php
- https://github.com/sonata-project/SonataDoctrineMongoDBAdminBundle/blob/master/Builder/FormContractor.php
There was a problem hiding this comment.
getDefaultOptions has some differences.
I guess this is mostly for historical reasons, i.e. changes not being systematically ported to all bundles.
The phpcr admin bundle is the only one having meaningful differences (outside the underlying metadata implementation details I mean).
That's why I proposed an AbstractFormContractor class in #3971
There was a problem hiding this comment.
Lol there is a __construct() method in FormContractorInterface
greg0ire
force-pushed
the
fix_field_description_doc
branch
3 times, most recently
from
87c20a1
to
3d28da6
Compare
|
What is the state of this PR @greg0ire? Anything else to do? |
| * @param AdminInterface $admin | ||
| * @param FieldDescriptionInterface $fieldDescription | ||
| * Adds missing information to the given field description from the model manager metadata, | ||
| * and the given admin. |
There was a problem hiding this comment.
Please keep it one-lined. This is under 120 chars.
greg0ire
force-pushed
the
fix_field_description_doc
branch
from
3d28da6
to
a4de665
Compare
This should give a better idea of what this method is supposed to be doing.
greg0ire
force-pushed
the
fix_field_description_doc
branch
from
a4de665
to
e7c2213
Compare
Nope. You can merge that IMO. |
|
Thank you @greg0ire |
Subject
This should give a better idea of what this method is supposed to be
doing.