Relative admin form is broken after 3.84.0

[vv12131415]

PHP version

PHP 7.4.9 (cli) (built: Aug  7 2020 14:29:10) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.9, Copyright (c), by Zend Technologies

Subject

I'm sure that it was in 3.84.0 and specifically cf1ec4d

Minimal repository with the bug

I'll provide one if the code that I'll give, will not be understandable

Steps to reproduce

0. You want to edit admin from related admin
1. Create one, general UserAdmin
2. Then create related UserInfoAdmin (the relations between tables is one-to-one)
   2.1. in configureFormFields of UserAdmin write $form->add('userInfo',AdminType::class,)
   2.2 The the user_info table has relation to one-to-many table called user_info_color (one info has many colors)
3. In UserInfoAdmin in configureFormFields set

            ->add(
                'colors',
                CollectionType::class,
                [],
                [
                    'edit' => 'inline',
                    'inline' => 'table',
                ]
            )

4. Open UserAdmin in web and try to set colors with
   
5. See

Error:
Call to a member function getValue() on null

  at vendor/sonata-project/admin-bundle/src/Admin/AdminHelper.php:228
  at Sonata\AdminBundle\Admin\AdminHelper->appendFormFieldElement(object(UserAdmin), object(User), 's6002dde957cdd_fakePublicInfo_tradeVolumes')
     (vendor/sonata-project/admin-bundle/src/Action/AppendFormFieldElementAction.php:77)
  at Sonata\AdminBundle\Action\AppendFormFieldElementAction->__invoke(object(Request))
     (vendor/symfony/http-kernel/HttpKernel.php:158)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
     (vendor/symfony/http-kernel/HttpKernel.php:80)
  at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
     (vendor/symfony/http-kernel/Kernel.php:201)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (Web/index.php:27)  

where the error happens

            // retrieve the posted data
            $data = $admin->getRequest()->get($formBuilder->getName());

Expected results

I get new line in my admin panel

Actual results

Error:
Call to a member function getValue() on null

  at vendor/sonata-project/admin-bundle/src/Admin/AdminHelper.php:228
  at Sonata\AdminBundle\Admin\AdminHelper->appendFormFieldElement(object(UserAdmin), object(User), 's6002dde957cdd_fakePublicInfo_tradeVolumes')
     (vendor/sonata-project/admin-bundle/src/Action/AppendFormFieldElementAction.php:77)
  at Sonata\AdminBundle\Action\AppendFormFieldElementAction->__invoke(object(Request))
     (vendor/symfony/http-kernel/HttpKernel.php:158)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
     (vendor/symfony/http-kernel/HttpKernel.php:80)
  at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
     (vendor/symfony/http-kernel/Kernel.php:201)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (Web/index.php:27)  

where the error happens

            // retrieve the posted data
            $data = $admin->getRequest()->get($formBuilder->getName());

---

the why it happens is because of this commit cf1ec4d in file src/Admin/AdminHelper.php method getChildFormBuilder. If I revert changes of this method, it works perfectly.

Should I make revert commit?

[VincentLanglet]

The error you get means that this line is returning null https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Admin/AdminHelper.php#L225

Can you debug a little more @vladyslavstartsev ; what's the difference between before and after the commit.
Maybe previous to this commit the code executed was the if part

SonataAdminBundle/src/Admin/AdminHelper.php

Line 205 in 12586a7

if (!$childFormBuilder) {

and now it's the else part.
Indeed, now the grandChildFormBuilder is found, but then their is no fieldDescription on the direct admin. The comment

SonataAdminBundle/src/Admin/AdminHelper.php

Line 203 in 12586a7

//Child form not found (probably nested one)

makes me believe that I'm right.

It seems weird to revert because if you look at getChildFormView it's recursive too. So it would be weird to have getChildFormBuilder non-recursive. By the way, these two method should be in another service (and could be static).

So maybe we could fix appendFormFieldElement instead. Either we modify the condition to stay in the if part in your case ; either we modify the code in the else part.

[vv12131415]

Yes, in 3.83, the $childFormBuilder is null
and in 3.84 it exists.

---

So maybe we could fix appendFormFieldElement instead.

Either we modify the condition to stay in the if part in your case ;

If we do the this one, it would not be recursive.

either we modify the code in the else part.

to modify code here, we need to get $fieldDescription right, not null (currently, 3.87 it's null), but FieldDescriptionInterface.
To get it not null, we need to make changes in $admin->getFormFieldDescription method.
Currently, if you call $admin->getFormFieldDescriptions(), you will only get one level of field descriptions.
So, there are 2 ways to get it

1. to recursively flatten fields of this admin to one level. But this way we will need to use some kind of prefixed or something else (because 2 admins can have 2 same fields).
2. to get all fields, but in nested way (nested array)
3. to call appendFormFieldElement of AdminHelper with not the parent admin, but with child one.

So, what is the best way to do it?

[VincentLanglet]

So maybe we could fix appendFormFieldElement instead.

Either we modify the condition to stay in the if part in your case ;

If we do the this one, it would not be recursive.

I don't understand.
If we replace this https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Admin/AdminHelper.php#L205
by

if (!$chilforbuilder || !$admin->hasFormFieldDescription($childFormBuilder->getName()))

it could work.

either we modify the code in the else part.

to modify code here, we need to get $fieldDescription right, not null (currently, 3.87 it's null), but FieldDescriptionInterface.
To get it not null, we need to make changes in $admin->getFormFieldDescription method.
Currently, if you call $admin->getFormFieldDescriptions(), you will only get one level of field descriptions.
So, there are 2 ways to get it

1. to recursively flatten fields of this admin to one level. But this way we will need to use some kind of prefixed or something else (because 2 admins can have 2 same fields).
2. to get all fields, but in nested way (nested array)
3. to call appendFormFieldElement of AdminHelper with not the parent admin, but with child one.

So, what is the best way to do it?

Dunno...

@franmomu You made the change on getChildFormBuilder, any opinion ?

[vv12131415]

yes,

if (!$chilforbuilder || !$admin->hasFormFieldDescription($childFormBuilder->getName()))

works perfectly.

[VincentLanglet]

yes,

if (!$chilforbuilder || !$admin->hasFormFieldDescription($childFormBuilder->getName()))

works perfectly.

This is the shorter fix we can do ; do you mind creating a PR with an unit test ?

[vv12131415]

yes, I'm working on it right now

[franmomu]

The change I made was to fix the prefix attribute and to make it recursive, if it should not, I think it's better to redone it in a non-recursive way.

To be honest I'm not familiar with the appendFormFieldElement code, looks like it needs some refactoring, at least to make it more understandable. So I'm fine with the solution proposed.