Access to CollectionType options in AdminType by VincentLanglet · Pull Request #6438 · sonata-project/SonataAdminBundle

VincentLanglet · 2020-10-01T19:37:31Z

I need to know the CollectionType value of by_reference in the AdminType.

The options are passed to the AdminType here:
https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/src/Builder/FormContractor.php#L172-L180

So I need to change the signature of getDefaultOptions to allow passing the formOptions.

But then I have two solutions:

Option 1: Adding a new symfony_form_options option to the AdminType, then in the FormContractor I will add

$options['symfony_form_options'] => $formOptions

And then I access to by_reference with $options['symfony_form_options']['by_reference'] ?? true.

Option 2: Using the by_reference option of the AdminType, then in the FormContractor I will add

if (isset($formOption['by_reference'])) {
    $options['by_reference'] => $formOptions['by_reference'];
}

And then I access to by_reference in the AdminType with $options['by_reference'].

WDYT @sonata-project/contributors ?

In this PR I have implemented the code for the solution 1, but passing all the options seems overkill to me and I think the solution 2 more elegant.

Changelog

### Fixed
- AdminType with CollectionType passed by reference

wbloszyk · 2020-10-04T13:25:05Z

IMO When you exnteds AbstractType then you should have symfony options directly in $options. Also in case you can add own options to $options or `$options['sonata'] and never vice versa (never $options['symfony']). This will make usage more logic.

I vote for option 2 😆

SonataCI · 2020-10-04T14:43:36Z

Could you please rebase your PR and fix merge conflicts?

src/Builder/FormContractorInterface.php

src/Form/Type/AdminType.php

VincentLanglet · 2020-10-14T23:15:14Z

Please review this bug fix @sonata-project/contributors

composer.json

src/Form/Type/AdminType.php

jordisala1991 · 2020-10-19T07:57:20Z

Tests are failing now

VincentLanglet · 2020-10-19T09:46:51Z

Fixed @jordisala1991

tests/Form/Type/AdminTypeTest.php

franmomu · 2020-10-20T18:21:06Z

src/Form/FormMapper.php

            // 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),


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.

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.

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

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

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

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.

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.

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

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?

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.

VincentLanglet · 2020-10-23T07:18:10Z

@franmomu Can we merge this bugfix and decide after if we need and how to refactor getDefaultOptions ?

VincentLanglet · 2020-10-23T09:05:20Z

Please review @sonata-project/contributors

VincentLanglet requested a review from a team October 1, 2020 19:37

VincentLanglet mentioned this pull request Oct 1, 2020

Admin with children forms does not associate entities #6426

Closed

SonataCI added pending author and removed pending author labels Oct 4, 2020

VincentLanglet mentioned this pull request Oct 4, 2020

Pass option by_reference in case we're using AdminType sonata-project/SonataDoctrineORMAdminBundle#1142

Merged

core23 reviewed Oct 8, 2020

View reviewed changes

src/Builder/FormContractorInterface.php Show resolved Hide resolved

VincentLanglet marked this pull request as ready for review October 12, 2020 19:17

VincentLanglet commented Oct 12, 2020

View reviewed changes

src/Form/Type/AdminType.php Outdated Show resolved Hide resolved

VincentLanglet requested a review from a team October 12, 2020 19:19

VincentLanglet requested review from jordisala1991 and phansys October 15, 2020 21:55

greg0ire added the patch label Oct 17, 2020

jordisala1991 reviewed Oct 19, 2020

View reviewed changes

composer.json Show resolved Hide resolved

src/Form/Type/AdminType.php Outdated Show resolved Hide resolved

VincentLanglet requested a review from a team October 19, 2020 06:49

jordisala1991 previously approved these changes Oct 19, 2020

View reviewed changes

VincentLanglet dismissed jordisala1991’s stale review via fba5507 October 19, 2020 08:14

VincentLanglet requested a review from a team October 19, 2020 09:47

phansys reviewed Oct 20, 2020

View reviewed changes

tests/Form/Type/AdminTypeTest.php Outdated Show resolved Hide resolved

Use addInstance if the collection is passed by reference

9b093b3

VincentLanglet requested review from jordisala1991 and phansys October 20, 2020 07:21

wbloszyk approved these changes Oct 20, 2020

View reviewed changes

franmomu reviewed Oct 20, 2020

View reviewed changes

VincentLanglet requested review from a team and franmomu October 20, 2020 19:40

VincentLanglet mentioned this pull request Oct 22, 2020

3.78 #6524

Merged

phansys approved these changes Oct 23, 2020

View reviewed changes

VincentLanglet merged commit bb27aa7 into sonata-project:3.x Oct 23, 2020

VincentLanglet deleted the wip branch October 23, 2020 12:19

jordisala1991 mentioned this pull request Oct 25, 2020

Remove fieldDescription deprecation sonata-project/SonataDoctrineORMAdminBundle#1170

Merged

VincentLanglet mentioned this pull request Nov 3, 2020

Admin embedded collection seems to remove entities with by_reference=false #6564

Closed

Uh oh!

Conversation

VincentLanglet commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Uh oh!

wbloszyk commented Oct 4, 2020

Uh oh!

SonataCI commented Oct 4, 2020

Uh oh!

Uh oh!

Uh oh!

VincentLanglet commented Oct 14, 2020

Uh oh!

Uh oh!

Uh oh!

jordisala1991 commented Oct 19, 2020

Uh oh!

VincentLanglet commented Oct 19, 2020

Uh oh!

Uh oh!

franmomu Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VincentLanglet Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VincentLanglet commented Oct 23, 2020

Uh oh!

VincentLanglet commented Oct 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

VincentLanglet commented Oct 1, 2020 •

edited

Loading

franmomu Oct 20, 2020 •

edited

Loading

VincentLanglet Oct 20, 2020 •

edited

Loading