Add support for FQCN form type referencing to FormContractor#196
Add support for FQCN form type referencing to FormContractor#196core23 merged 1 commit intosonata-project:3.xfrom
Conversation
pascal-hofmann
force-pushed
the
3.x
branch
2 times, most recently
from
4bbb4cb to
e41f0e0
Compare
|
I've added tests and changed comparison of form types. Comparison is now done like in SonataDoctrineORMAdminBundle. |
|
Nice coverage increase! Please fix the failing test though |
|
For fixed test case see my other PR. The failing test is not caused by my changes, so I made a separate PR. Will rebase this one when the other one is merged. |
|
Perfect |
|
Rebased on latest 3.x. Tests should work now. |
|
Would be great to have this merged! is merging done by SonataCI automatically? |
Tests/Builder/FormContractorTest.php
Outdated
| use Sonata\DoctrineMongoDBAdminBundle\Builder\FormContractor; | ||
| use Symfony\Component\Form\FormFactoryInterface; | ||
|
|
||
| final class FormContractorTest extends \PHPUnit_Framework_TestCase |
There was a problem hiding this comment.
No reason to make this final, it's just an internal test class
|
The policy is to have 2 approvals before merging |
|
|
||
| /** | ||
| * @var FormFactoryInterface | ||
| */ |
There was a problem hiding this comment.
Is there a reason why this is protected instead of private?
There was a problem hiding this comment.
FormContractor is not final, and subclasses may want to access $formFactory.
There was a problem hiding this comment.
'may' want.,.
Please make this private until someone really need this
There was a problem hiding this comment.
I don't see a reason to limit extensibility. $fieldFactory is protected too. And FormContractor of SonataDoctrineORMBundle has these entities protected too.
There was a problem hiding this comment.
On the other hand, it's accessible through the getter... so no reason to not change it. I'll update the PR.
There was a problem hiding this comment.
There was a problem hiding this comment.
Understood. Valid arguments. Thank you!
Is the updated PR OK for you now @core23?
|
Thanks @pascal-hofmann |
5 participants
I am targetting this branch, because this is a bugfix that allows using the new FQCN form type references (i.e.
ModelType::class) instead of referencing by type name (i.e.'sonata_type_model').Closes sonata-project/SonataAdminBundle#3976 and /pull/178
Changelog
To do
Subject
FormContractor::getDefaultOptionsdidn't check against form types FQCNs, causing the default options not to be set when using the Symfony 3 way:$formMapper->add('foo', ModelType::class);See related admin-bundle issue at sonata-project/SonataAdminBundle#3976