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.

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

Infinite Recursion when using fields with 'help' option #6142

Closed
jtaylor100 opened this issue Jun 15, 2020 · 9 comments · Fixed by #6143 or #6148
Closed

Infinite Recursion when using fields with 'help' option #6142

jtaylor100 opened this issue Jun 15, 2020 · 9 comments · Fixed by #6143 or #6148
Assignees

Comments

@jtaylor100
Copy link
Contributor

Environment

  • Ubuntu 20.04
  • Symfony 4.4
  • PHP 7.4

Sonata packages

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.69.0 3.69.0 The missing Symfony Admin Generator
sonata-project/block-bundle              3.19.0 4.2.0  Symfony SonataBlockBundle
sonata-project/cache                     2.0.1  2.0.1  Cache library
sonata-project/core-bundle               3.20.0 3.20.0 Symfony SonataCoreBundle (abandoned)
sonata-project/doctrine-extensions       1.6.0  1.6.0  Doctrine2 behavioral extensions
sonata-project/doctrine-orm-admin-bundle 3.18.0 3.18.0 Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/exporter                  2.2.0  2.2.0  Lightweight Exporter library
sonata-project/form-extensions           0.1.2  1.4.0  Symfony form extensions
sonata-project/twig-extensions           0.1.1  1.3.0  Sonata twig extensions

Symfony packages

$ composer show --latest 'symfony/*'
Restricting packages listed in "symfony/symfony" to "4.4.*"
symfony/asset                      v4.4.10 v4.4.10 Symfony Asset Component
symfony/browser-kit                v4.4.10 v4.4.10 Symfony BrowserKit Component
symfony/cache                      v4.4.10 v4.4.10 Symfony Cache component with PSR-6, PSR-16, and tags
symfony/cache-contracts            v2.1.2  v2.1.2  Generic abstractions related to caching
symfony/config                     v4.4.10 v4.4.10 Symfony Config Component
symfony/console                    v4.4.10 v4.4.10 Symfony Console Component
symfony/css-selector               v4.4.10 v4.4.10 Symfony CssSelector Component
symfony/debug                      v4.4.10 v4.4.10 Symfony Debug Component
symfony/debug-bundle               v4.4.10 v4.4.10 Symfony DebugBundle
symfony/dependency-injection       v4.4.10 v4.4.10 Symfony DependencyInjection Component
symfony/deprecation-contracts      v2.1.2  v2.1.2  A generic function and convention to trigger deprecation notices
symfony/doctrine-bridge            v4.4.10 v4.4.10 Symfony Doctrine Bridge
symfony/dom-crawler                v4.4.10 v4.4.10 Symfony DomCrawler Component
symfony/error-handler              v4.4.10 v4.4.10 Symfony ErrorHandler Component
symfony/event-dispatcher           v4.4.10 v4.4.10 Symfony EventDispatcher Component
symfony/event-dispatcher-contracts v1.1.7  v2.1.2  Generic abstractions related to dispatching event
symfony/expression-language        v4.4.10 v4.4.10 Symfony ExpressionLanguage Component
symfony/filesystem                 v4.4.10 v4.4.10 Symfony Filesystem Component
symfony/finder                     v4.4.10 v4.4.10 Symfony Finder Component
symfony/form                       v4.4.10 v4.4.10 Symfony Form Component
symfony/framework-bundle           v4.4.10 v4.4.10 Symfony FrameworkBundle
symfony/http-foundation            v4.4.10 v4.4.10 Symfony HttpFoundation Component
symfony/http-kernel                v4.4.10 v4.4.10 Symfony HttpKernel Component
symfony/inflector                  v4.4.10 v4.4.10 Symfony Inflector Component
symfony/intl                       v4.4.10 v4.4.10 A PHP replacement layer for the C intl extension that includes additional data from the ICU library.
symfony/mime                       v4.4.10 v4.4.10 A library to manipulate MIME messages
symfony/monolog-bridge             v4.4.10 v4.4.10 Symfony Monolog Bridge
symfony/monolog-bundle             v3.5.0  v3.5.0  Symfony MonologBundle
symfony/options-resolver           v4.4.10 v4.4.10 Symfony OptionsResolver Component
symfony/phpunit-bridge             v5.1.1  v5.1.1  Symfony PHPUnit Bridge
symfony/polyfill-ctype             v1.17.0 v1.17.0 Symfony polyfill for ctype functions
symfony/polyfill-intl-grapheme     v1.17.0 v1.17.0 Symfony polyfill for intl's grapheme_* functions
symfony/polyfill-intl-idn          v1.17.0 v1.17.0 Symfony polyfill for intl's idn_to_ascii and idn_to_utf8 functions
symfony/polyfill-intl-normalizer   v1.17.0 v1.17.0 Symfony polyfill for intl's Normalizer class and related functions
symfony/polyfill-php73             v1.17.0 v1.17.0 Symfony polyfill backporting some PHP 7.3+ features to lower PHP versions
symfony/polyfill-php80             v1.17.0 v1.17.0 Symfony polyfill backporting some PHP 8.0+ features to lower PHP versions
symfony/process                    v4.4.10 v4.4.10 Symfony Process Component
symfony/property-access            v4.4.10 v4.4.10 Symfony PropertyAccess Component
symfony/property-info              v4.4.10 v4.4.10 Symfony Property Info Component
symfony/proxy-manager-bridge       v4.4.10 v4.4.10 Symfony ProxyManager Bridge
symfony/psr-http-message-bridge    v2.0.0  v2.0.0  PSR HTTP message bridge
symfony/routing                    v4.4.10 v4.4.10 Symfony Routing Component
symfony/security                   v4.4.10 v4.4.10 Symfony Security Component
symfony/security-acl               v3.0.4  v3.0.4  Symfony Security Component - ACL (Access Control List)
symfony/security-bundle            v4.4.10 v4.4.10 Symfony SecurityBundle
symfony/serializer                 v4.4.10 v4.4.10 Symfony Serializer Component
symfony/service-contracts          v2.1.2  v2.1.2  Generic abstractions related to writing services
symfony/stopwatch                  v4.4.10 v4.4.10 Symfony Stopwatch Component
symfony/string                     v5.1.1  v5.1.1  Symfony String component
symfony/swiftmailer-bundle         v3.2.4  v3.4.0  Symfony SwiftmailerBundle
symfony/templating                 v4.4.10 v4.4.10 Symfony Templating Component
symfony/translation                v4.4.10 v4.4.10 Symfony Translation Component
symfony/translation-contracts      v2.1.2  v2.1.2  Generic abstractions related to translation
symfony/twig-bridge                v4.4.10 v4.4.10 Symfony Twig Bridge
symfony/twig-bundle                v4.4.10 v4.4.10 Symfony TwigBundle
symfony/validator                  v4.4.10 v4.4.10 Symfony Validator Component
symfony/var-dumper                 v4.4.10 v4.4.10 Symfony mechanism for exploring and dumping PHP variables
symfony/var-exporter               v4.4.10 v4.4.10 A blend of var_export() + serialize() to turn any serializable data structure to plain PHP code
symfony/web-profiler-bundle        v4.4.10 v4.4.10 Symfony WebProfilerBundle
symfony/webpack-encore-bundle      v1.7.3  v1.7.3  Integration with your Symfony app & Webpack Encore!
symfony/yaml                       v4.4.10 v4.4.10 Symfony Yaml Component

PHP version

$ php -v
PHP 7.4.7 (cli) (built: Jun 12 2020 07:47:45) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0, Copyright (c) Zend Technologies
    with Zend OPcache v7.4.7, Copyright (c), by Zend Technologies
    with Xdebug v2.9.6, Copyright (c) 2002-2020, by Derick Rethans

Subject

After upgrading to v3.69.0, we started to experience

PHP Fatal error:  Allowed memory size of 2147483648 bytes exhausted...

while requesting many of our sonata admin pages. Breaking with the debugger before this memory exhaustion occurred showed this stack trace, suggesting infinite recursion when trying to map form fields.

... etc ...
(Proprietary Admin)->configureFormFields()
AbstractAdmin.php:1381, (Proprietary Admin)->defineFormBuilder()
AbstractAdmin.php:1354, (Proprietary Admin)->getFormBuilder()
AbstractAdmin.php:3360, (Proprietary Admin)->buildForm()
AbstractAdmin.php:1825, (Proprietary Admin)->getFormFieldDescription()
FormMapper.php:143, Sonata\AdminBundle\Form\FormMapper->add()
(Proprietary Admin)->configureFormFields()
AbstractAdmin.php:1381, (Proprietary Admin)->defineFormBuilder()
AbstractAdmin.php:1354, (Proprietary Admin)->getFormBuilder()
AbstractAdmin.php:3360, (Proprietary Admin)->buildForm()
AbstractAdmin.php:1825, (Proprietary Admin)->getFormFieldDescription()
FormMapper.php:143, Sonata\AdminBundle\Form\FormMapper->add()
(Proprietary Admin)->configureFormFields()
AbstractAdmin.php:1381, (Proprietary Admin)->defineFormBuilder()
AbstractAdmin.php:1354, (Proprietary Admin)->getFormBuilder()
AbstractAdmin.php:3360, (Proprietary Admin)->buildForm()
AbstractAdmin.php:1429, (Proprietary Admin)->getForm()
CRUDController.php:311, Sonata\AdminBundle\Controller\CRUDController->editAction()
HttpKernel.php:158, Symfony\Component\HttpKernel\HttpKernel->handleRaw()
HttpKernel.php:80, Symfony\Component\HttpKernel\HttpKernel->handle()
Kernel.php:201, TestAppKernel->handle()
... PHPUnit Client ...

After commenting out the help options on our form fields, then the errors did not occur. Stepping through the code, getFormFieldDescription is only called if the 'help' option is included.

Potentially related to #6104?

Steps to reproduce

Create an admin with a form element that has help text.

Expected results

Successful page load.

Actual results

Out of memory exceptions.

@VincentLanglet
Copy link
Member

VincentLanglet commented Jun 15, 2020

@jtaylor100 Thanks for the report.

So

  • editAction call admin->getForm()
  • getForm call $this->buildForm();
  • buildForm call $this->getFormBuilder()
  • getFormBuilder call $this->defineFormBuilder
  • defineFormBuilder call $this->configureFormFields

Inside, you're using formMapper->add, with the following code

if (null !== $help) {
    $this->admin->getFormFieldDescription($name)->setHelp($help);
}

And getFormFieldDescription is calling $this->buildForm.

IMHO, we have two strategy here:

  • Remove the $this->buildForm(); call in both getFormFieldDescriptions, getFormFieldDescription and hasFormFieldDescription. Same for Show and List.

    • This seems to be a good strategy because calling a hasser/getter should modify the existing form and shouldn't have such impact on the admin and calling all these methods.
    • On the other side, you can't get the form field descriptions if the form is not build. So if we stop building the form, we should throw an exception when it's not already build. And have a public method to build it.

In the RetrieveAutocompleteItemsAction.php, we can see the problem.

    private function retrieveFormFieldDescription(
        AdminInterface $admin,
        string $field
    ): FieldDescriptionInterface {
        $admin->getFormFieldDescriptions(); // This is a "hack" to build the form

        $fieldDescription = $admin->getFormFieldDescription($field); // We can now access to the fieldDescription

        ...
    }
  • Fix the way the help option is set.

Instead of

$this->admin->addFormFieldDescription($fieldName, $fieldDescription);

And then

if (null !== $help) {
    $this->admin->getFormFieldDescription($name)->setHelp($help);
}

We could try to write somthing like

if (null !== $help) {
    fieldDescription->setHelp($help);
}
$this->admin->addFormFieldDescription($fieldName, $fieldDescription);

The first solution would need some works and some debate

  • Remove the calls buildForm in getFormFieldDescriptions, getFormFieldDescription, hasFormFieldDescription.
  • Do we do the same in the getForm method ?
  • Add some deprecation/exception when you call these methods when the form is not build.
  • Do we expose the buidForm method ?
  • Manually call buildForm before using getForm, getFormFieldDescriptions, ...
  • Doing the same for Show and List

The second solution is easier and could be considered as a quick fix ; do you want to try the PR ?

@jtaylor100
Copy link
Contributor Author

Sure thing, please assign me. I will try out the second solution.

@gremo
Copy link
Contributor

gremo commented Jun 17, 2020

This should be fixed in 3.69.1 but I still have problems (memory exhausted) with SonataPageBundle (page admin). Can you confirm that the fix is not working yet?

The problem goes away if I comment https://github.com/sonata-project/SonataAdminBundle/blob/3.x/src/Form/FormMapper.php#L260 so it's still related to form help, isn't it?

@VincentLanglet
Copy link
Member

I think you're right @gremo

It's coming from here:
https://github.com/sonata-project/SonataPageBundle/blob/f1c0d7a1dd06c31b541e4cd9ab857be019ae723c/src/Admin/PageAdmin.php#L399

Seems like it's really too risky to keep the buildShow, buildForm, buildList methods everywhere.

IMHO, this PR could be the right way to fix the issue #6148, but we need to check if it breaks something else and how to fix it.

As shown by the test https://github.com/sonata-project/SonataAdminBundle/blob/3.x/tests/Admin/AdminTest.php#L1809, the modelAdmin has no filterDescription when we try to access them without building the Datagrid.

But it would make no sense to build it in the getFilterFieldDescriptions method and not getFilterFieldDescription like it was before. It leads to this weird code, using the getFilterFieldDescriptions method to build the datagrid and then getFilterFieldDescription to access the value:

$admin->getFilterFieldDescriptions();
$fieldDescription = $admin->getFilterFieldDescription($field);

We maybe should replace the current getXXXFieldDescriptions by buildXXX. But this method is currently not public.

@gremo
Copy link
Contributor

gremo commented Jun 17, 2020

@VincentLanglet thanks for the explanation, you are clearly more involved than me into this 👍

I'm working on a new porject and I need to get this fixed: I can downgrade sonata-admin, do you know which version is not affected (which commit break the help feature)? Thanks!

@jtaylor100
Copy link
Contributor Author

@gremo For us the issues began with the upgrade from 3.68.0 => 3.69.0

So I would downgrade to 3.68.0 and locking until the issue is resolved for you.

And for the record, the fix in #6143 has fixed all our (@customer-alliance) regressions and we no longer experience any page loading issues.

@VincentLanglet
Copy link
Member

@gremo For us the issues began with the upgrade from 3.68.0 => 3.69.0

So I would downgrade to 3.68.0 and locking until the issue is resolved for you.

I agree, the risk of creating infinite loop was always here ; but the bug you found was added in 3.69.

And for the record, the fix in #6143 has fixed all our (@customer-alliance) regressions and we no longer experience any page loading issues.

It fixed the issue for SonataAdmin but not for SonataPage.
In reality, it fix the issue as long as you don't call one of the methods

  • $this->getFormFieldDescriptions()
  • $this->getFormFieldDescription()
  • $this->hasFormFieldDescription()
    in the configureFormField method. But SonataPage does.

This is too easy for user to create infinite loop, that's why we need a long term fix.
I'll try with #6148

@gremo
Copy link
Contributor

gremo commented Jun 17, 2020

@jtaylor100 @VincentLanglet thank you both, i'll try to downgrade now.

@VincentLanglet
Copy link
Member

VincentLanglet commented Jun 18, 2020

@jtaylor100 @gremo I think I have found a solution, If you want to take a look
#6148

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment