Skip to content

Access to CollectionType options in AdminType #6438

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

Merged
VincentLanglet merged 1 commit into from
Oct 23, 2020
Merged

Access to CollectionType options in AdminType #6438

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@
"doctrine/doctrine-bundle": ">=3",
"sonata-project/core-bundle": "<3.20",
"sonata-project/doctrine-extensions": "<1.8",
"sonata-project/doctrine-mongodb-admin-bundle": "<3.5",
"sonata-project/doctrine-orm-admin-bundle": "<3.24",
Comment thread
jordisala1991 marked this conversation as resolved.
"sonata-project/media-bundle": "<3.7",
"sonata-project/user-bundle": "<3.3"
},
Expand Down
6 changes: 6 additions & 0 deletions phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,12 @@ parameters:
count: 1
path: src/Command/CreateClassCacheCommand.php

-
# will be fixed in v4. Currently BC break
message: "#^Method Sonata\\\\AdminBundle\\\\Builder\\\\FormContractorInterface\\:\\:getDefaultOptions\\(\\) invoked with 3 parameters, 2 required\\.$#"
count: 1
path: src/Form/FormMapper.php

# next 6 errors are due to not installed Doctrine ORM\ODM
-
message: "#^Class Doctrine\\\\ODM\\\\MongoDB\\\\PersistentCollection not found\\.$#"
Expand Down
6 changes: 4 additions & 2 deletions src/Builder/FormContractorInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,16 @@ public function __construct(FormFactoryInterface $formFactory);
*
* @return FormBuilderInterface
*/
public function getFormBuilder($name, array $options = []);
public function getFormBuilder($name, array $formOptions = []);

/**
* NEXT_MAJOR: Change signature to add the third parameter $formOptions.
*
* Should provide Symfony form options.
*
* @param string|null $type
*
* @return array
*/
public function getDefaultOptions($type, FieldDescriptionInterface $fieldDescription);
public function getDefaultOptions($type, FieldDescriptionInterface $fieldDescription/*, array $formOptions = []*/);
Comment thread
VincentLanglet marked this conversation as resolved.
}
5 changes: 4 additions & 1 deletion src/Form/FormMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,10 @@ public function add($name, $type = null, array $options = [], array $fieldDescri
$name = $fieldDescription->getName();

// Note that the builder var is actually the formContractor:
$options = array_replace_recursive($this->builder->getDefaultOptions($type, $fieldDescription) ?? [], $options);
$options = array_replace_recursive(
$this->builder->getDefaultOptions($type, $fieldDescription, $options),
Copy link
Copy Markdown
Member

@franmomu franmomu Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably I'm missing something, but why do we need to pass the form options to the builder (persistence bundles) if we already have them here?

Apparently https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1142/files this just adds the by_reference to the array.

Copy link
Copy Markdown
Member Author

@VincentLanglet VincentLanglet Oct 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they are transformed.

From

$options = ['by_reference' => true];

To

$options = [`type_options` => ['by_reference' => true]]

(And multiple others modifications)

And the options passed are not always the same ; the depends on the type, etc
It the responsibility of the persistence bundle to re-format the options.

Copy link
Copy Markdown
Member

@phansys phansys Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember some unintended behavior regarding these options (#6078).

Copy link
Copy Markdown
Member Author

@VincentLanglet VincentLanglet Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure to understand. ^^'
Is it an answer to franmomu ? A request change to me ?

Copy link
Copy Markdown
Member

@phansys phansys Oct 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. The comment is regarding the modifications you've mentioned about the options.

Copy link
Copy Markdown
Member

@phansys phansys Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning seems fine to me.
I just have a concern. Since we are adding a 3rd argument in order to fix this issue properly in the next major, shouldn't be better to provide the fix by using this new argument through func_get_args() and deprecate the case where it's considered required and not used?
I didn't see how this new argument will be used in the method, so in case my previous concern can not be done, I think at least we should leave the comment with the code fragment that must be uncommented in the next major.

Copy link
Copy Markdown
Member Author

@VincentLanglet VincentLanglet Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand @phansys.

To me, the only NEXT_MAJOR comment needed is https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Builder/FormContractor.php#L101

The 3rd argument will always be used from now, so I don't understand why you want to deprecate it ?
This is a long term fix.

Copy link
Copy Markdown
Member

@phansys phansys Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 3rd argument will always be used from now, so I don't understand why you want to deprecate it ?

I'm not saying we should deprecate the argument, please re-read my comment:

deprecate the case where it's considered required and not used

Copy link
Copy Markdown
Member

@phansys phansys Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, the only NEXT_MAJOR comment needed is https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Builder/FormContractor.php#L101

Sorry, didn't see that the call to func_get_args() and the NEXT_MAJOR comment were already there before this PR.

I think then we should just trigger a deprecation if the FormContractor::getDefaultOptions() method is called without the 3rd argument. Do you think it could be done in this PR?

Copy link
Copy Markdown
Member Author

@VincentLanglet VincentLanglet Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getDefaultOptions is implemented in persistence bundle, so I can't do it in this SonataAdmin PR.

But I can create PR on persistence bundle then.

$options
);

// be compatible with mopa if not installed, avoid generating an exception for invalid option
// force the default to false ...
Expand Down
8 changes: 7 additions & 1 deletion src/Form/Type/AdminType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,13 @@ static function (array $associationMapping): string {
$subject = $p->getValue($parentSubject, $parentPath.$path);
} catch (NoSuchIndexException $e) {
// no object here, we create a new one
$subject = ObjectManipulator::setObject($admin->getNewInstance(), $parentSubject, $parentFieldDescription);
$subject = $admin->getNewInstance();

if ($options['by_reference']) {
$subject = ObjectManipulator::addInstance($parentSubject, $subject, $parentFieldDescription);
} else {
$subject = ObjectManipulator::setObject($subject, $parentSubject, $parentFieldDescription);
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions tests/Form/FormMapperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class FormMapperTest extends TestCase
protected function setUp(): void
{
$this->contractor = $this->createMock(FormContractorInterface::class);
$this->contractor->method('getDefaultOptions')->willReturn([]);

$formFactory = $this->createMock(FormFactoryInterface::class);
$eventDispatcher = $this->createMock(EventDispatcherInterface::class);
Expand Down
79 changes: 68 additions & 11 deletions tests/Form/Type/AdminTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ public function testSubmitValidData(): void
$foo = new Foo();

$admin = $this->createMock(AbstractAdmin::class);
$admin->expects($this->atLeastOnce())->method('hasParentFieldDescription')->willReturn(true);
$admin->expects($this->atLeastOnce())->method('getParentFieldDescription')->willReturn($parentField);
$admin->expects($this->exactly(2))->method('hasParentFieldDescription')->willReturn(true);
$admin->expects($this->exactly(2))->method('getParentFieldDescription')->willReturn($parentField);
$admin->expects($this->once())->method('hasAccess')->with('delete')->willReturn(false);
$admin->expects($this->once())->method('defineFormBuilder');
$admin->expects($this->once())->method('getModelManager')->willReturn($modelManager);
Expand Down Expand Up @@ -120,8 +120,8 @@ public function testDotFields(): void
$modelManager = $this->createStub(ModelManagerInterface::class);

$admin = $this->createMock(AbstractAdmin::class);
$admin->expects($this->atLeastOnce())->method('hasParentFieldDescription')->willReturn(true);
$admin->expects($this->atLeastOnce())->method('getParentFieldDescription')->willReturn($parentField);
$admin->expects($this->exactly(2))->method('hasParentFieldDescription')->willReturn(true);
$admin->expects($this->exactly(2))->method('getParentFieldDescription')->willReturn($parentField);
$admin->expects($this->once())->method('setSubject')->with(1);
$admin->expects($this->once())->method('defineFormBuilder');
$admin->expects($this->once())->method('getModelManager')->willReturn($modelManager);
Expand Down Expand Up @@ -162,8 +162,8 @@ public function testArrayCollection(): void
$modelManager = $this->createStub(ModelManagerInterface::class);

$admin = $this->createMock(AbstractAdmin::class);
$admin->expects($this->atLeastOnce())->method('hasParentFieldDescription')->willReturn(true);
$admin->expects($this->atLeastOnce())->method('getParentFieldDescription')->willReturn($parentField);
$admin->expects($this->exactly(2))->method('hasParentFieldDescription')->willReturn(true);
$admin->expects($this->exactly(2))->method('getParentFieldDescription')->willReturn($parentField);
$admin->expects($this->once())->method('defineFormBuilder');
$admin->expects($this->once())->method('getModelManager')->willReturn($modelManager);
$admin->expects($this->once())->method('getClass')->willReturn(Foo::class);
Expand All @@ -189,8 +189,64 @@ public function testArrayCollection(): void

public function testArrayCollectionNotFound(): void
{
$parentSubject = new \stdClass();
$parentSubject->foo = new ArrayCollection([]);
$parentSubject = new class() {
public $foo = [];
};

$parentAdmin = $this->createMock(AdminInterface::class);
$parentAdmin->expects($this->once())->method('getSubject')->willReturn($parentSubject);
$parentAdmin->expects($this->once())->method('hasSubject')->willReturn(true);
$parentField = $this->createMock(FieldDescriptionInterface::class);
$parentField->expects($this->once())->method('setAssociationAdmin')->with($this->isInstanceOf(AdminInterface::class));
$parentField->expects($this->once())->method('getAdmin')->willReturn($parentAdmin);
$parentField->expects($this->once())->method('getParentAssociationMappings')->willReturn([]);
$parentField->expects($this->once())->method('getAssociationMapping')->willReturn(['fieldName' => 'foo', 'mappedBy' => 'bar']);

$modelManager = $this->createStub(ModelManagerInterface::class);

$newInstance = new class() {
public function setBar()
{
}
};

$admin = $this->createMock(AbstractAdmin::class);
$admin->expects($this->exactly(2))->method('hasParentFieldDescription')->willReturn(true);
$admin->expects($this->exactly(2))->method('getParentFieldDescription')->willReturn($parentField);
$admin->expects($this->once())->method('defineFormBuilder');
$admin->expects($this->once())->method('getModelManager')->willReturn($modelManager);
$admin->expects($this->once())->method('getClass')->willReturn(Foo::class);
$admin->expects($this->once())->method('setSubject')->with($newInstance);
$admin->expects($this->once())->method('getNewInstance')->willReturn($newInstance);

$field = $this->createMock(FieldDescriptionInterface::class);
$field->expects($this->once())->method('getAssociationAdmin')->willReturn($admin);
$field->expects($this->atLeastOnce())->method('getFieldName')->willReturn('foo');
$field->expects($this->once())->method('getParentAssociationMappings')->willReturn([]);

$this->builder->add('foo');

try {
$this->adminType->buildForm($this->builder, [
'sonata_field_description' => $field,
'delete' => false, // not needed
'property_path' => '[0]', // actual test case
'by_reference' => false,
]);
} catch (NoSuchPropertyException $exception) {
$this->fail($exception->getMessage());
}
}

public function testArrayCollectionByReferenceNotFound(): void
{
$parentSubject = new class() {
public $foo = [];

public function addFoo()
{
}
};

$parentAdmin = $this->createMock(AdminInterface::class);
$parentAdmin->expects($this->once())->method('getSubject')->willReturn($parentSubject);
Expand All @@ -203,11 +259,11 @@ public function testArrayCollectionNotFound(): void

$modelManager = $this->createStub(ModelManagerInterface::class);

$newInstance = new Foo();
$newInstance = new \stdClass();

$admin = $this->createMock(AbstractAdmin::class);
$admin->expects($this->atLeastOnce())->method('hasParentFieldDescription')->willReturn(true);
$admin->expects($this->atLeastOnce())->method('getParentFieldDescription')->willReturn($parentField);
$admin->expects($this->exactly(2))->method('hasParentFieldDescription')->willReturn(true);
$admin->expects($this->exactly(2))->method('getParentFieldDescription')->willReturn($parentField);
$admin->expects($this->once())->method('defineFormBuilder');
$admin->expects($this->once())->method('getModelManager')->willReturn($modelManager);
$admin->expects($this->once())->method('getClass')->willReturn(Foo::class);
Expand All @@ -226,6 +282,7 @@ public function testArrayCollectionNotFound(): void
'sonata_field_description' => $field,
'delete' => false, // not needed
'property_path' => '[0]', // actual test case
'by_reference' => true,
]);
} catch (NoSuchPropertyException $exception) {
$this->fail($exception->getMessage());
Expand Down