Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix iterating over children in form builder #6694

Merged
merged 3 commits into from
Dec 11, 2020
Merged

Fix iterating over children in form builder #6694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ac7094e
Fix iterating over children in form builder
franmomu Dec 10, 2020
28fe03b
Deprecate passing other type than string|null as prefix
franmomu Dec 10, 2020
47afea9
Change from createMock to createStub
franmomu Dec 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/Admin/AdminHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down
25 changes: 20 additions & 5 deletions src/Util/FormBuilderIterator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class FormBuilderIterator extends \RecursiveArrayIterator
protected $keys = [];

/**
* @var bool|string
* @var string
*/
protected $prefix;

Expand All @@ -48,13 +48,28 @@ 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)
public function __construct(FormBuilderInterface $formBuilder, $prefix = null)
{
parent::__construct();
$this->formBuilder = $formBuilder;
$this->prefix = $prefix ?: $formBuilder->getName();

// 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.
// $this->prefix = $prefix ?? $formBuilder->getName();
$this->iterator = new \ArrayIterator(self::getKeys($formBuilder));
}

Expand Down Expand Up @@ -87,7 +102,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()
Expand Down
47 changes: 31 additions & 16 deletions tests/Admin/AdminHelperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -89,6 +89,21 @@ public function testGetChildFormBuilder(): void
$this->assertInstanceOf(FormBuilder::class, $this->helper->getChildFormBuilder($formBuilder, 'test_elementId'));
}

public function testGetGrandChildFormBuilder(): void
{
$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);
$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();
Expand Down Expand Up @@ -220,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);
Expand All @@ -237,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([]);
Expand All @@ -252,7 +267,7 @@ public function testAppendFormFieldElement(): void
'bar' => $fieldDescription,
]);

$request = $this->createMock(Request::class);
$request = $this->createStub(Request::class);
$request
->method('get')
->willReturn([
Expand All @@ -270,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
Expand All @@ -288,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);
Expand Down Expand Up @@ -328,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);

Expand Down
27 changes: 21 additions & 6 deletions tests/Util/FormBuilderIteratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -25,6 +26,8 @@
*/
class FormBuilderIteratorTest extends TestCase
{
use ExpectDeprecationTrait;

/**
* @var EventDispatcherInterface
*/
Expand All @@ -42,9 +45,9 @@ class FormBuilderIteratorTest extends TestCase

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->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);
}

Expand All @@ -60,6 +63,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
Expand All @@ -68,8 +72,19 @@ public function testHasChildren(): void
$iterator = new FormBuilderIterator($this->builder);
$this->assertTrue($iterator->hasChildren());
}
}

class TestFormBuilder extends FormBuilder
{
/**
* 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');
}
}