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

Stop building show/list/form when accessing fieldDescription #6148

Merged
merged 3 commits into from
Jun 19, 2020

Conversation

VincentLanglet
Copy link
Member

@VincentLanglet VincentLanglet commented Jun 17, 2020

Subject

I am targeting this branch, because I think I have found a BC-way to fix the bug (#6142) which can easily occur and create infinite loop.

Currently methods getFormFieldDescriptions, getFormFieldDescription and hasFormFieldDescription are calling this->buildForm.
But the buildForm method is calling lot of things, as shown in the issue, we may find

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

in the FormMapper->add, or

$formMapper->setHelps(['name' => 'help_page_name']);

in the SonataPageBundle configureFormField method witch lead to a hasFormFieldDescriptions call.

The developper could also possibly use the getFormFieldDescriptions, getFormFieldDescription and hasFormFieldDescription in the configureFormField and create an infinite loop.

Before we didn't try to build the form if the form was already build.
By adding a state, I now do not try to build the form if the build of the form already started.

Closes #6142.

Changelog

### Fixed
- `getFormFieldDescriptions`, `getFormFieldDescription` and `hasFormFieldDescription` doesn't build form anymore if the build already started, avoiding an infinite loop.
- `getShowFieldDescriptions`, `getShowFieldDescription` and `hasShowFieldDescription` doesn't build show anymore if the build already started, avoiding an infinite loop.
- `getListFieldDescriptions`, `getListFieldDescription` and `hasListFieldDescription` doesn't build list anymore if the build already started, avoiding an infinite loop.
- `getFilterFieldDescriptions`, `getFilterFieldDescription` and `hasFilterFieldDescription` doesn't build datagrid anymore if the build already started, avoiding an infinite loop.

@VincentLanglet
Copy link
Member Author

@sonata-project/contributors I need your help on this !

@jordisala1991
Copy link
Member

If the issue is not easily fixable, isnt it better to just revert your pr and then try to find a better way? to avoid breaking peoples apps.

@VincentLanglet
Copy link
Member Author

If the issue is not easily fixable, isnt it better to just revert your pr and then try to find a better way? to avoid breaking peoples apps.

Yes, if I don't find better solution I will remove the buildXXX method only in getFormFieldDescription.

But the issue will still be present for others methods, it just not currently happen.

Currently SonataPageBundle call getFormFieldDescription('toto') during the configureFormField ; luckily it doesn't build the form. But if the call was made this way getFormFieldDescriptions['toto'] it would have create the same issue.

But I think I have find the right way to fix this definitely by adding a state.
I build the form only if I've not already started to build the form.
The same strategy was used for the menu or the routes.

@VincentLanglet VincentLanglet marked this pull request as ready for review June 18, 2020 06:50
jordisala1991
jordisala1991 previously approved these changes Jun 18, 2020
@jordisala1991 jordisala1991 requested a review from a team June 18, 2020 07:04
@jordisala1991
Copy link
Member

Looks ok to me, @gremo does it fix your issue?

core23
core23 previously approved these changes Jun 18, 2020
Copy link
Member

@core23 core23 left a comment

Choose a reason for hiding this comment

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

No tests required?

@VincentLanglet
Copy link
Member Author

You want a functionnal test to prevent the infinite loop @core23 ?

@VincentLanglet VincentLanglet dismissed stale reviews from core23 and jordisala1991 via a433ad2 June 18, 2020 20:29
jordisala1991
jordisala1991 previously approved these changes Jun 18, 2020
@VincentLanglet
Copy link
Member Author

Done @core23.

It was pretty useful since I caught a mistake. I wrote

$this->loaded['form']

instead of

$this->loaded['form'] = true

so the PR wasn't fixing the bug...

But everything is ok now.

jordisala1991
jordisala1991 previously approved these changes Jun 18, 2020
@phansys phansys added the patch label Jun 19, 2020
tests/Admin/AdminTest.php Outdated Show resolved Hide resolved
tests/Admin/AdminTest.php Outdated Show resolved Hide resolved
tests/Admin/AdminTest.php Show resolved Hide resolved
tests/Admin/AdminTest.php Outdated Show resolved Hide resolved
src/Admin/AdminInterface.php Outdated Show resolved Hide resolved
src/Admin/AdminInterface.php Outdated Show resolved Hide resolved
src/Admin/AdminInterface.php Outdated Show resolved Hide resolved
src/Admin/AdminInterface.php Outdated Show resolved Hide resolved
src/Admin/AdminInterface.php Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Member Author

Done @phansys

@core23 core23 merged commit 5386609 into sonata-project:3.x Jun 19, 2020
@core23
Copy link
Member

core23 commented Jun 19, 2020

Thanks @VincentLanglet

@VincentLanglet VincentLanglet deleted the removeBuild branch June 19, 2020 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite Recursion when using fields with 'help' option
4 participants