[FORM] Prevent forms from extending itself as a parent #24387

pierredup · 2017-10-01T08:11:38Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	N/A
License	MIT
Doc PR	N/A

If a form type defines itself as a parent, it results in an endless recursive loop in the form registry, so throw an exception instead.

ogizanagi · 2017-10-01T08:48:29Z

Thanks for the PR, but I wouldn't qualify this as a bug. As an enhancement, so a new feature, this must target 4.1, where you'll use the form type FQCN rather than the name btw :)

pierredup · 2017-10-01T09:08:07Z

I had this issue on 3.4 with FOSUserBundle when I copy-pasted some code from the docs (https://symfony.com/doc/master/bundles/FOSUserBundle/overriding_forms.html), but my class name was the same as one of the core bundle class names (I changed the class strings to RegistrationFormType::class without adding a use alias, so it resolved to the same class name as my form).
As a result, the page just hanged until it timed out, and there was no information in the logs (with xdebug, you would at least see the max-nesting level error in the logs, but I had xdebug disabled at that point). It took a while to debug and figure out that what was happening. So IMHO I don't think this is a new feature. Instead of ending in an endless recursive loop with no idea what is happening and no information given to the user that they did something wrong, we can exit early with a message that gives some more context about why the page suddenly bombs out.

ogizanagi · 2017-10-01T09:13:40Z

Sure, I understand that. But that's not an issue within the form component itself, so it cannot be part of a patch release. This PR is a DX improvement. We're used to qualify those as features, and it lands on new minor versions.

Simperfit · 2017-10-01T09:22:07Z

I agree with @ogizanagi's statement, this is not a bug fix but a DX improvement. It allows users to better use the component. Maybe it needs a doc entry with a warning when you do that ?

nicolas-grekas · 2017-10-01T09:26:22Z

While this changes a behavior, I think the feature freeze period is fine for this kind of changes, where no new feature is really introduced (no new public anything).
Saving debugging time is of primary importance to me.

nicolas-grekas · 2017-10-01T09:28:22Z

Preventing an infinite loop also could legitimately qualify as a bug fix to me, so it's not obvious it must go on 4.1.

ogizanagi · 2017-10-01T09:34:34Z

So talking a bit with @nicolas-grekas, and as it does not only enhance DX but prevents an infinite loop, I agree this must be qualified as a bug fix.

yceruto · 2017-10-01T19:45:42Z

src/Symfony/Component/Form/Tests/Fixtures/FormWithSameParentTypeAsObject.php

+
+    public function getParent()
+    {
+        return new FormWithSameParentTypeAsObject();


Please, use return new self(); to make fabbot happy again ;)

I did consider changing this initially when I saw the failure, but I'm just concerned it would make the test less intuitive

Why would it look less intuitive to you? Would be even more explicit to me :)

Could you do the change?

ogizanagi

except Fabbot must be happy

stof · 2017-10-02T12:25:58Z

What about more complex infinite loops ? A -> B -> A instead of just A -> A ?

nicolas-grekas · 2017-10-08T07:55:52Z

src/Symfony/Component/Form/FormRegistry.php

+        static $checkedTypes = array();
+
+        $searchKey = array_search($type->getName(), $checkedTypes);
+        $checkedTypes[] = $type->getName();


would putting the name as key work instead?

If the name is the key, then you won't get the proper dependency path when throwing an error,
E.G if you have the dependencies foo -> bar -> foo then you can only have the values foo > bar in the array that is displayed in the message, where we want to show foo > bar > foo (So we want to be able to have duplicate values in the array)

Is it possible to have duplicate values and not have a circular ref? If not, then shouldn't this be just the terminal condition? (ie the last > foo comes from the current call, not from the checkedTypes stack?)

You're right, any duplicate value would immediately be the cause of the circular ref. I have updated this

nicolas-grekas · 2017-10-08T10:07:30Z

src/Symfony/Component/Form/FormRegistry.php

@@ -97,13 +98,33 @@ public function getType($name)
     */
    private function resolveAndAddType(FormTypeInterface $type)
    {
+        static $checkedTypes = array();


does this need to be static, or can it be a regular private property?
also, what about replacing the $checkedTypes = array(); reset by a (method-wide) finally?
That'd be more robust I think

Initially when I had it as a private property, it didn't work and still ended in an infinite loop, but I must have been doing something wrong since it's working fine now :)
Also this still needs to support PHP 5.3, so we can't use finally

nicolas-grekas · 2017-10-08T12:56:21Z

src/Symfony/Component/Form/FormRegistry.php

        $parentType = $type->getParent();

        if ($parentType instanceof FormTypeInterface) {
+            if ($parentType->getName() === $type->getName()) {
+                $this->checkedTypes = array();


I think we should still use a try/catch emulating finally, so that the code can recover from any unexpected exception

nicolas-grekas

(with one minor comment)

nicolas-grekas · 2017-10-09T08:53:48Z

(please mind fabbot also)

fabpot · 2017-10-10T04:33:32Z

Sorry, but this does not qualify as a bug fix. It's a nice DX improvement for sure, but it does not fix anything (as having an infinite loop is a dev error, nothing wrong with the code). I would merge this in 3.4 instead as a new feature (which by the way is we added some similar circular refs checks in the past).

pierredup · 2017-10-10T08:31:22Z

Rebased against 3.4

nicolas-grekas · 2017-10-10T08:50:53Z

src/Symfony/Component/Form/FormRegistry.php

+                unset($this->checkedTypes[$fqcn]);
+            }
+        } finally {
+            $this->checkedTypes = array();


I think this is buggy: there should be only one try/finally, it should start right after $this->checkedTypes[$fqcn] = true;, and should only unset($this->checkedTypes[$fqcn]); (doing $this->checkedTypes = array() could be possible, but only on exceptions - but is not required if we unset properly)

nicolas-grekas · 2017-10-10T09:03:36Z

src/Symfony/Component/Form/FormRegistry.php

+        $this->checkedTypes[$fqcn] = true;
+
+        try {
+            if ($parentType === $fqcn) {


one last thing sorry: this "if" can be moved before L117, isn't it?

I'm wondering if this is even necessary anymore? Without it, you would still get the circular ref error. Or is it better to be explicit in checking the same type as the parent?

let's remove it then you're right

nicolas-grekas · 2017-10-10T10:07:30Z

Thank you @pierredup.

… (pierredup) This PR was squashed before being merged into the 3.4 branch (closes #24387). Discussion ---------- [FORM] Prevent forms from extending itself as a parent | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A If a form type defines itself as a parent, it results in an endless recursive loop in the form registry, so throw an exception instead. Commits ------- 2ee6fd5 [FORM] Prevent forms from extending itself as a parent

stof · 2017-10-10T10:52:46Z

src/Symfony/Component/Form/FormRegistry.php

-            $typeExtensions = array_merge(
+        if (isset($this->checkedTypes[$fqcn])) {
+            $types = implode(' > ', array_merge(array_keys($this->checkedTypes), array($fqcn)));
+            throw new LogicException(sprintf('Circular reference detected for form "%s" (%s).', $fqcn, $types));


it is detected for a form type, not for a form.

…ies check (pierredup) This PR was squashed before being merged into the 3.4 branch (closes #24504). Discussion ---------- [Form] Fix error message in circular reference dependencies check | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | | License | MIT | Doc PR | Fix comments from PR #24387 Commits ------- 48eb43d [Form] Fix error message in circular reference dependencies check

carsonbot added Status: Needs Review Form Bug labels Oct 1, 2017

ogizanagi added this to the 4.1 milestone Oct 1, 2017

ogizanagi added Feature and removed Bug labels Oct 1, 2017

ogizanagi modified the milestones: 4.1, 2.7 Oct 1, 2017

ogizanagi added Bug and removed Feature labels Oct 1, 2017

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 1, 2017

Simperfit approved these changes Oct 1, 2017

View reviewed changes

yceruto reviewed Oct 1, 2017

View reviewed changes

ogizanagi approved these changes Oct 2, 2017

View reviewed changes

pierredup force-pushed the form-parents branch from 6ce04fe to 513b238 Compare October 3, 2017 07:23

nicolas-grekas reviewed Oct 8, 2017

View reviewed changes

nicolas-grekas approved these changes Oct 9, 2017

View reviewed changes

pierredup added 9 commits October 10, 2017 10:08

Prevent forms from having itself as a parent

38f4f83

Return new self instead of class name

5d33c27

Handle circular form dependencies

9d7cb13

Add form name as key to checkedTypes

c1bf6cb

Replace static variable with private property

04d2661

Clear array in exception handler

a7f2dfc

Fix CS

104de31

Update to use FQCN for form types

d41c159

Removed unused classes

73cc6d6

pierredup changed the base branch from 2.7 to 3.4 October 10, 2017 08:27

pierredup force-pushed the form-parents branch from a867fde to 73cc6d6 Compare October 10, 2017 08:28

pierredup added 2 commits October 10, 2017 10:29

Fix CS

746f1d5

Removed unused test class

c9ed200

nicolas-grekas modified the milestones: 2.7, 3.4 Oct 10, 2017

nicolas-grekas reviewed Oct 10, 2017

View reviewed changes

Remove additional try/finally

d2b027c

nicolas-grekas reviewed Oct 10, 2017

View reviewed changes

Remove explicit parent form type check

c8aac13

nicolas-grekas approved these changes Oct 10, 2017

View reviewed changes

nicolas-grekas closed this Oct 10, 2017

stof reviewed Oct 10, 2017

View reviewed changes

pierredup mentioned this pull request Oct 10, 2017

[Form] Fix error message in circular reference dependencies check #24504

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

pierredup deleted the form-parents branch August 28, 2018 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FORM] Prevent forms from extending itself as a parent #24387

[FORM] Prevent forms from extending itself as a parent #24387

pierredup commented Oct 1, 2017 •

edited

Loading

ogizanagi commented Oct 1, 2017

pierredup commented Oct 1, 2017 •

edited

Loading

ogizanagi commented Oct 1, 2017 •

edited

Loading

Simperfit commented Oct 1, 2017

nicolas-grekas commented Oct 1, 2017 •

edited

Loading

nicolas-grekas commented Oct 1, 2017

ogizanagi commented Oct 1, 2017

yceruto Oct 1, 2017

pierredup Oct 2, 2017

ogizanagi Oct 2, 2017

ogizanagi left a comment •

edited

Loading

stof commented Oct 2, 2017

nicolas-grekas Oct 8, 2017

pierredup Oct 8, 2017 •

edited

Loading

nicolas-grekas Oct 8, 2017 •

edited

Loading

pierredup Oct 8, 2017

nicolas-grekas Oct 8, 2017 •

edited

Loading

pierredup Oct 8, 2017

nicolas-grekas Oct 8, 2017

nicolas-grekas left a comment

nicolas-grekas commented Oct 9, 2017

fabpot commented Oct 10, 2017

pierredup commented Oct 10, 2017

nicolas-grekas Oct 10, 2017

nicolas-grekas Oct 10, 2017 •

edited

Loading

pierredup Oct 10, 2017 •

edited

Loading

nicolas-grekas Oct 10, 2017

nicolas-grekas commented Oct 10, 2017

stof Oct 10, 2017

[FORM] Prevent forms from extending itself as a parent #24387

[FORM] Prevent forms from extending itself as a parent #24387

Conversation

pierredup commented Oct 1, 2017 • edited Loading

ogizanagi commented Oct 1, 2017

pierredup commented Oct 1, 2017 • edited Loading

ogizanagi commented Oct 1, 2017 • edited Loading

Simperfit commented Oct 1, 2017

nicolas-grekas commented Oct 1, 2017 • edited Loading

nicolas-grekas commented Oct 1, 2017

ogizanagi commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi left a comment • edited Loading

Choose a reason for hiding this comment

stof commented Oct 2, 2017

Choose a reason for hiding this comment

pierredup Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Oct 9, 2017

fabpot commented Oct 10, 2017

pierredup commented Oct 10, 2017

Choose a reason for hiding this comment

nicolas-grekas Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

pierredup Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Oct 10, 2017

Choose a reason for hiding this comment

pierredup commented Oct 1, 2017 •

edited

Loading

pierredup commented Oct 1, 2017 •

edited

Loading

ogizanagi commented Oct 1, 2017 •

edited

Loading

nicolas-grekas commented Oct 1, 2017 •

edited

Loading

ogizanagi left a comment •

edited

Loading

pierredup Oct 8, 2017 •

edited

Loading

nicolas-grekas Oct 8, 2017 •

edited

Loading

nicolas-grekas Oct 8, 2017 •

edited

Loading

nicolas-grekas Oct 10, 2017 •

edited

Loading

pierredup Oct 10, 2017 •

edited

Loading