[Form] handle callable strings in PropertyAccessDecorator #18057

HeahDude · 2016-03-08T10:25:04Z

Q	A
Branch	2.7+
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	-

…ecorator ref symfony#17993. Handle edge cases where a callable string is passed to the `PropertyAccessDecorator` for choice list factories. Wrapping the callable string in a closure prevents a potential fatal error, since it may be called with the wrong arguments.

HeahDude · 2016-03-08T10:25:17Z

ref #17993.

HeahDude · 2016-03-09T05:38:09Z

src/Symfony/Component/Form/ChoiceList/Factory/PropertyAccessDecorator.php

@@ -172,6 +172,11 @@ public function createView(ChoiceListInterface $list, $preferredChoices = null,

        if (is_string($label) && !is_callable($label)) {
            $label = new PropertyPath($label);
+        } elseif (is_string($label) && is_callable($label)) {


Maybe this should be the responsibility of the DefaultChoiceListFactory as the interface is supposed to support callable anyway.

Then it would just need to make PropertyAccessDecorator treat callable strings as strings, instead of passing them to the decorated factory as it is.

But one using the DefaultChoiceListFactory without decorator should not experience this bug.

What do you think @webmozart ?

webmozart · 2016-03-09T09:48:49Z

Thanks for working on this PR @HeahDude! :) Why do we need to add this? I would like to stick with a deprecation error here (which can be added into the default factory, yes).

HeahDude · 2016-03-10T16:24:01Z

@webmozart I got confused by your comment.

It is now very clear after reading @Tobion's #17993 (comment).

The purpose of #15561 is exactly why I said:

Maybe this should be the responsibility of the DefaultChoiceListFactory as the interface is supposed to support callable anyway.

as you agreed (what confused me the most).

But I was not aware that it was a "fix" introduced by #15561.
However I think this fix is responsible for revealing the bug of the fatal error because the options handled in createView() cannot currently support string callables.
And also responsible for a regression in PropertyAccessDecorator motivating #17993.

So here's my suggestion:

Commit 1 - Bugfix - 2.7 - PropertyAccessDecorator - revert 470b140:
When using the PropertyAccessDecorator strings should have a precedence on callables.
This is also what Tobion's comment suggests:

If you think we should revert that, we could just do it as a hotfix as well I assume.

In that case one wanting to use a callable string should be responsible for wrapping it in a closure.
Commit 2 - Bugfix - 2.7 - DefaultChoiceListFactory - fix back [Form] group_by does not works with callable string #15542:
This PR should target the DefaultChoiceListFactory as it should handle string callables and be responsible for the wrapping since string callables are valid callables.
[Form] group_by does not works with callable string #15542 needs to be fixed for factories that don't use the PropertyAccessDecorator in IMHO.
Then if commit 1 is merged there is no need for deprecation.
Otherwise both the interface and the docs should mention that string callables are not supported.
Commit 3 - Deprecate - 3.1:
If commit 2 is merged and you don't want to support it anymore fix #17993 - Deprecated callable strings #18020 should add deprecations in the default factory when wrapping the callable and throw an error instead in 4.0 plus should modify the interface documentation.

xabbuh · 2016-03-11T12:11:56Z

I can see one reason why this might be useful: Let's say you start a project where you a string that is no callable and thus is treated as a property path. Then, some time later due to an update or whatever a global function with the same name as the string used here is added. Now, the application would fail, wouldn't it? With this fix we would be able to avoid such failures if I am not mistaken.

HeahDude · 2016-03-11T16:28:24Z

@xabbuh Yes however this PR is wrong as it is as this should not be the responsibility of the PropertyAccessDecorator and I'm not sure this fix will not introduce another issue...

I don't think we should add any extra support for string callables. It was never intended to support this functionality.

I now understand it, I've failed before because I thought string callables were common callables and the DefaultChoiceList was supposed to support them.

That would mean that to properly fix it, we should be aware of the signature to make a difference between #15542's example 'MyStatic::method' and 'strtolower' (or 'end') since the first may handle the right arguments and the second will end with an error when calling call_user_func.

The fix of this PR contradict the current behavior and would just make the opposite happens: the first would fail as it may expect three arguments and the proposed "fix" closure only pass one.

This is the same concern for any type of callable so one should just stick to the doc to know what arguments the provided callable should deal with and be aware that by default the PropertyAccessDecorator has no precedence on strings until in 4.0.
Hence the ChoiceType default factory treats for now its the option values as callable before "string to property path" (deprecated in #18020).

Then, some time later due to an update or whatever a global function with the same name as the string used here is added.

To prevent the case you're mentioning or deal with existing function until 4.0 is out, one should pass a property path directly:

'option_label' => new PropertyPath('end'),

which make me think that @weaverryan response to symfony/symfony-docs#6282 (comment) might be reconsidered, and maybe we should precise it for concerned options.

Anyway closing here.

stof · 2016-03-25T10:44:21Z

src/Symfony/Component/Form/ChoiceList/Factory/PropertyAccessDecorator.php

@@ -172,6 +172,11 @@ public function createView(ChoiceListInterface $list, $preferredChoices = null,

        if (is_string($label) && !is_callable($label)) {
            $label = new PropertyPath($label);
+        } elseif (is_string($label) && is_callable($label)) {
+            // Prevent a fatal error since a callable string may not handle the right arguments


How does it prevent a fatal error ? You call it with the same argument

@stof Actually choice_label, choice_name, choice_attr, group_by and preferred_choices as callable get passed 3 arguments ($choice, $key, $value) and we got a fatal error with callable such as 'end' but I found out that was not valid because 'MyStatic::getMeChoiceName' would then fail if it needs 3 arguments, so this is a "won't fix" and I closed this PR.

This PR was merged into the 3.1-dev branch. Discussion ---------- fix #17993 - Deprecated callable strings | Q | A | ------------- | --- | Branch | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | no | Fixed tickets | #17993 | License | MIT | Doc PR | Is this ok ? - [x] Rebase when #18057 is merged Commits ------- 191b495 fix #17993 - Deprecated callable strings

carsonbot added the Status: Needs Review label Mar 8, 2016

javiereguiluz added Form Bug labels Mar 8, 2016

HeahDude mentioned this pull request Mar 9, 2016

fix #17993 - Deprecated callable strings #18020

Merged

1 task

HeahDude reviewed Mar 9, 2016
View reviewed changes

HeahDude closed this Mar 11, 2016

HeahDude deleted the fix-property_access_decorator branch March 11, 2016 16:28

xabbuh mentioned this pull request Mar 11, 2016

[Form] fix choice_label values symfony/symfony-docs#6282

Merged

stof reviewed Mar 25, 2016
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] handle callable strings in PropertyAccessDecorator #18057

[Form] handle callable strings in PropertyAccessDecorator #18057

[Form] handle callable strings in PropertyAccessDecorator #18057

[Form] handle callable strings in PropertyAccessDecorator #18057

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment