From ac7094ea15ed4e076aea07aab2fea8b85c677875 Mon Sep 17 00:00:00 2001 From: Fran Moreno Date: Thu, 10 Dec 2020 20:12:10 +0100 Subject: [PATCH 1/3] Fix iterating over children in form builder AdminHelper::getChildFormBuilder() method was iterating only over the first level of form builder. It is needed a RecursiveIteratorIterator for a tree traversal as AdminHelper::getChildFormView() method does. It also fixes FormBuilderIterator prefix property when traversing children, now the current prefix is passed to the children to create its key. --- src/Admin/AdminHelper.php | 9 +++++++-- src/Util/FormBuilderIterator.php | 13 +++++++++---- tests/Admin/AdminHelperTest.php | 15 +++++++++++++++ tests/Util/FormBuilderIteratorTest.php | 7 ++----- 4 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/Admin/AdminHelper.php b/src/Admin/AdminHelper.php index 01fceffe33..aa8d87d3f2 100644 --- a/src/Admin/AdminHelper.php +++ b/src/Admin/AdminHelper.php @@ -102,9 +102,14 @@ public function __construct($poolOrPropertyAccessor) */ public function getChildFormBuilder(FormBuilderInterface $formBuilder, $elementId) { - foreach (new FormBuilderIterator($formBuilder) as $name => $formBuilder) { + $iterator = new \RecursiveIteratorIterator( + new FormBuilderIterator($formBuilder), + \RecursiveIteratorIterator::SELF_FIRST + ); + + foreach ($iterator as $name => $currentFormBuilder) { if ($name === $elementId) { - return $formBuilder; + return $currentFormBuilder; } } diff --git a/src/Util/FormBuilderIterator.php b/src/Util/FormBuilderIterator.php index e3545db091..a16083a2cd 100644 --- a/src/Util/FormBuilderIterator.php +++ b/src/Util/FormBuilderIterator.php @@ -38,7 +38,7 @@ class FormBuilderIterator extends \RecursiveArrayIterator protected $keys = []; /** - * @var bool|string + * @var string */ protected $prefix; @@ -48,13 +48,18 @@ class FormBuilderIterator extends \RecursiveArrayIterator protected $iterator; /** - * @param bool $prefix + * NEXT_MAJOR: Change argument 2 to ?string $prefix = null. + * + * @param string|false $prefix */ public function __construct(FormBuilderInterface $formBuilder, $prefix = false) { parent::__construct(); $this->formBuilder = $formBuilder; - $this->prefix = $prefix ?: $formBuilder->getName(); + // NEXT_MAJOR: Remove next line. + $this->prefix = \is_string($prefix) ? $prefix : $formBuilder->getName(); + // NEXT_MAJOR: Uncomment next line. + // $this->prefix = $prefix ?? $formBuilder->getName(); $this->iterator = new \ArrayIterator(self::getKeys($formBuilder)); } @@ -87,7 +92,7 @@ public function current() public function getChildren() { - return new self($this->formBuilder->get($this->iterator->current()), $this->current()); + return new self($this->formBuilder->get($this->iterator->current()), $this->key()); } public function hasChildren() diff --git a/tests/Admin/AdminHelperTest.php b/tests/Admin/AdminHelperTest.php index e28c6716af..cff0c57bb8 100644 --- a/tests/Admin/AdminHelperTest.php +++ b/tests/Admin/AdminHelperTest.php @@ -89,6 +89,21 @@ public function testGetChildFormBuilder(): void $this->assertInstanceOf(FormBuilder::class, $this->helper->getChildFormBuilder($formBuilder, 'test_elementId')); } + public function testGetGrandChildFormBuilder(): void + { + $formFactory = $this->createMock(FormFactoryInterface::class); + $eventDispatcher = $this->createMock(EventDispatcherInterface::class); + + $formBuilder = new FormBuilder('parent', \stdClass::class, $eventDispatcher, $formFactory); + $childFormBuilder = new FormBuilder('child', \stdClass::class, $eventDispatcher, $formFactory); + $grandchildFormBuilder = new FormBuilder('grandchild', \stdClass::class, $eventDispatcher, $formFactory); + + $formBuilder->add($childFormBuilder); + $childFormBuilder->add($grandchildFormBuilder); + + $this->assertInstanceOf(FormBuilder::class, $this->helper->getChildFormBuilder($formBuilder, 'parent_child_grandchild')); + } + public function testGetChildFormView(): void { $formView = new FormView(); diff --git a/tests/Util/FormBuilderIteratorTest.php b/tests/Util/FormBuilderIteratorTest.php index bade580507..ff15f7708c 100644 --- a/tests/Util/FormBuilderIteratorTest.php +++ b/tests/Util/FormBuilderIteratorTest.php @@ -44,7 +44,7 @@ protected function setUp(): void { $this->dispatcher = $this->getMockForAbstractClass(EventDispatcherInterface::class); $this->factory = $this->getMockForAbstractClass(FormFactoryInterface::class); - $this->builder = new TestFormBuilder('name', null, $this->dispatcher, $this->factory); + $this->builder = new FormBuilder('name', null, $this->dispatcher, $this->factory); $this->factory->method('createNamedBuilder')->willReturn($this->builder); } @@ -60,6 +60,7 @@ public function testGetChildren(): void $this->builder->add('name', TextType::class); $iterator = new FormBuilderIterator($this->builder); $this->assertInstanceOf(\get_class($iterator), $iterator->getChildren()); + $this->assertSame('name_name', $iterator->key()); } public function testHasChildren(): void @@ -69,7 +70,3 @@ public function testHasChildren(): void $this->assertTrue($iterator->hasChildren()); } } - -class TestFormBuilder extends FormBuilder -{ -} From 28fe03bbee2aa445320dea813dc26a6187bf416a Mon Sep 17 00:00:00 2001 From: Fran Moreno Date: Thu, 10 Dec 2020 20:44:39 +0100 Subject: [PATCH 2/3] Deprecate passing other type than string|null as prefix --- src/Util/FormBuilderIterator.php | 12 +++++++++++- tests/Admin/AdminHelperTest.php | 4 ++-- tests/Util/FormBuilderIteratorTest.php | 18 ++++++++++++++++++ 3 files changed, 31 insertions(+), 3 deletions(-) diff --git a/src/Util/FormBuilderIterator.php b/src/Util/FormBuilderIterator.php index a16083a2cd..3c97587bc5 100644 --- a/src/Util/FormBuilderIterator.php +++ b/src/Util/FormBuilderIterator.php @@ -52,10 +52,20 @@ class FormBuilderIterator extends \RecursiveArrayIterator * * @param string|false $prefix */ - public function __construct(FormBuilderInterface $formBuilder, $prefix = false) + public function __construct(FormBuilderInterface $formBuilder, $prefix = null) { parent::__construct(); $this->formBuilder = $formBuilder; + + // NEXT_MAJOR: Remove this block. + if (null !== $prefix && !\is_string($prefix)) { + @trigger_error(sprintf( + 'Passing other type than string or null as argument 2 for method %s() is deprecated since' + .' sonata-project/admin-bundle 3.x. It will accept only string and null in version 4.0.', + __METHOD__ + ), E_USER_DEPRECATED); + } + // NEXT_MAJOR: Remove next line. $this->prefix = \is_string($prefix) ? $prefix : $formBuilder->getName(); // NEXT_MAJOR: Uncomment next line. diff --git a/tests/Admin/AdminHelperTest.php b/tests/Admin/AdminHelperTest.php index cff0c57bb8..f9a657fac5 100644 --- a/tests/Admin/AdminHelperTest.php +++ b/tests/Admin/AdminHelperTest.php @@ -91,8 +91,8 @@ public function testGetChildFormBuilder(): void public function testGetGrandChildFormBuilder(): void { - $formFactory = $this->createMock(FormFactoryInterface::class); - $eventDispatcher = $this->createMock(EventDispatcherInterface::class); + $formFactory = $this->createStub(FormFactoryInterface::class); + $eventDispatcher = $this->createStub(EventDispatcherInterface::class); $formBuilder = new FormBuilder('parent', \stdClass::class, $eventDispatcher, $formFactory); $childFormBuilder = new FormBuilder('child', \stdClass::class, $eventDispatcher, $formFactory); diff --git a/tests/Util/FormBuilderIteratorTest.php b/tests/Util/FormBuilderIteratorTest.php index ff15f7708c..6ff9a8c617 100644 --- a/tests/Util/FormBuilderIteratorTest.php +++ b/tests/Util/FormBuilderIteratorTest.php @@ -15,6 +15,7 @@ use PHPUnit\Framework\TestCase; use Sonata\AdminBundle\Util\FormBuilderIterator; +use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait; use Symfony\Component\EventDispatcher\EventDispatcherInterface; use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\FormBuilder; @@ -25,6 +26,8 @@ */ class FormBuilderIteratorTest extends TestCase { + use ExpectDeprecationTrait; + /** * @var EventDispatcherInterface */ @@ -69,4 +72,19 @@ public function testHasChildren(): void $iterator = new FormBuilderIterator($this->builder); $this->assertTrue($iterator->hasChildren()); } + + /** + * NEXT_MAJOR: Remove this test. + * + * @group legacy + */ + public function testTriggersADeprecationWithWrongPrefixType(): void + { + $this->expectDeprecation('Passing other type than string or null as argument 2 for method Sonata\AdminBundle\Util\FormBuilderIterator::__construct() is deprecated since sonata-project/admin-bundle 3.x. It will accept only string and null in version 4.0.'); + + $this->builder->add('name', TextType::class); + $iterator = new FormBuilderIterator($this->builder, new \stdClass()); + + $this->assertSame($iterator->key(), 'name_name'); + } } From 47afea9fcdbbf601b78623041112df04e69d66ef Mon Sep 17 00:00:00 2001 From: Fran Moreno Date: Fri, 11 Dec 2020 09:05:40 +0100 Subject: [PATCH 3/3] Change from createMock to createStub Changes mocks from stubs when these mocks do not have any expectation. --- tests/Admin/AdminHelperTest.php | 32 +++++++++++++------------- tests/Util/FormBuilderIteratorTest.php | 4 ++-- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/tests/Admin/AdminHelperTest.php b/tests/Admin/AdminHelperTest.php index f9a657fac5..cc5cc6a45c 100644 --- a/tests/Admin/AdminHelperTest.php +++ b/tests/Admin/AdminHelperTest.php @@ -77,8 +77,8 @@ public function testDeprecatedConstructingWithoutPropertyAccessor(): void public function testGetChildFormBuilder(): void { - $formFactory = $this->createMock(FormFactoryInterface::class); - $eventDispatcher = $this->createMock(EventDispatcherInterface::class); + $formFactory = $this->createStub(FormFactoryInterface::class); + $eventDispatcher = $this->createStub(EventDispatcherInterface::class); $formBuilder = new FormBuilder('test', \stdClass::class, $eventDispatcher, $formFactory); @@ -235,7 +235,7 @@ public function testAppendFormFieldElement(): void { $helper = new AdminHelper($this->propertyAccessor); - $admin = $this->createMock(AdminInterface::class); + $admin = $this->createStub(AdminInterface::class); $admin ->method('getClass') ->willReturn(Foo::class); @@ -252,7 +252,7 @@ public function testAppendFormFieldElement(): void 'isOwningSide' => false, ]; - $fieldDescription = $this->createMock(FieldDescriptionInterface::class); + $fieldDescription = $this->createStub(FieldDescriptionInterface::class); $fieldDescription->method('getAssociationAdmin')->willReturn($associationAdmin); $fieldDescription->method('getAssociationMapping')->willReturn($associationMapping); $fieldDescription->method('getParentAssociationMappings')->willReturn([]); @@ -267,7 +267,7 @@ public function testAppendFormFieldElement(): void 'bar' => $fieldDescription, ]); - $request = $this->createMock(Request::class); + $request = $this->createStub(Request::class); $request ->method('get') ->willReturn([ @@ -285,7 +285,7 @@ public function testAppendFormFieldElement(): void $admin ->method('getRequest') - ->will($this->onConsecutiveCalls($request, $request, $request, null, $request, $request, $request, $request, null, $request)); + ->willReturnOnConsecutiveCalls($request, $request, $request, null, $request, $request, $request, $request, null, $request); $foo = $this->createMock(Foo::class); $admin @@ -303,9 +303,9 @@ public function testAppendFormFieldElement(): void $foo->expects($this->atLeastOnce())->method('addBar')->with($bar); - $dataMapper = $this->createMock(DataMapperInterface::class); - $formFactory = $this->createMock(FormFactoryInterface::class); - $eventDispatcher = $this->createMock(EventDispatcherInterface::class); + $dataMapper = $this->createStub(DataMapperInterface::class); + $formFactory = $this->createStub(FormFactoryInterface::class); + $eventDispatcher = $this->createStub(EventDispatcherInterface::class); $formBuilder = new FormBuilder('test', \get_class($foo), $eventDispatcher, $formFactory); $childFormBuilder = new FormBuilder('bar', \stdClass::class, $eventDispatcher, $formFactory); $childFormBuilder->setCompound(true); @@ -343,21 +343,21 @@ public function testAppendFormFieldElementNested(): void { $admin = $this->createMock(AdminInterface::class); $object = $this->getMockBuilder(\stdClass::class) - ->setMethods(['getSubObject']) + ->addMethods(['getSubObject']) ->getMock(); $subObject = $this->getMockBuilder(\stdClass::class) - ->setMethods(['getAnd']) + ->addMethods(['getAnd']) ->getMock(); $sub2Object = $this->getMockBuilder(\stdClass::class) - ->setMethods(['getMore']) + ->addMethods(['getMore']) ->getMock(); $sub3Object = $this->getMockBuilder(\stdClass::class) - ->setMethods(['getFinalData']) + ->addMethods(['getFinalData']) ->getMock(); - $dataMapper = $this->createMock(DataMapperInterface::class); - $formFactory = $this->createMock(FormFactoryInterface::class); - $eventDispatcher = $this->createMock(EventDispatcherInterface::class); + $dataMapper = $this->createStub(DataMapperInterface::class); + $formFactory = $this->createStub(FormFactoryInterface::class); + $eventDispatcher = $this->createStub(EventDispatcherInterface::class); $formBuilder = new FormBuilder('test', \get_class($object), $eventDispatcher, $formFactory); $childFormBuilder = new FormBuilder('subObject', \get_class($subObject), $eventDispatcher, $formFactory); diff --git a/tests/Util/FormBuilderIteratorTest.php b/tests/Util/FormBuilderIteratorTest.php index 6ff9a8c617..679295a0b4 100644 --- a/tests/Util/FormBuilderIteratorTest.php +++ b/tests/Util/FormBuilderIteratorTest.php @@ -45,8 +45,8 @@ class FormBuilderIteratorTest extends TestCase protected function setUp(): void { - $this->dispatcher = $this->getMockForAbstractClass(EventDispatcherInterface::class); - $this->factory = $this->getMockForAbstractClass(FormFactoryInterface::class); + $this->dispatcher = $this->createStub(EventDispatcherInterface::class); + $this->factory = $this->createStub(FormFactoryInterface::class); $this->builder = new FormBuilder('name', null, $this->dispatcher, $this->factory); $this->factory->method('createNamedBuilder')->willReturn($this->builder); }