Skip to content

PropertyAccessDecorator should not accept callable strings #17993

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.

Sign up for GitHub

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

Jump to bottom
Closed
webmozart opened this issue Mar 3, 2016 · 2 comments
Closed

PropertyAccessDecorator should not accept callable strings #17993

webmozart opened this issue Mar 3, 2016 · 2 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Form Good first issue Ideal for your first contribution! (some Symfony experience may be required)

Comments

@webmozart
Copy link
Contributor

Right now, PropertyAccessDecorator treats callable strings as callables, not as strings. Consequently the following code does not behave as expected:

$builder->add('deadline', ChoiceType::class, [
    'choices' => $dateRanges, // DateRange objects
    'choice_label' => 'end',
]);

This code results in a fatal error. The end() function exists (is callable), hence the string is not treated as property path. Instead, end() is called with invalid arguments. IMO there are very few cases where this would be the expected behavior, and even if, it could be solved by passing a closure:

$builder->add('deadline', ChoiceType::class, [
    'choices' => $dateRanges, // arrays of dates
    'choice_label' => function (array $dateRange) {
        return end($dateRange);
    },
]);

Treating callable strings as callables should be deprecated. I.e., all methods of PropertyAccessDecorator should trigger deprecation errors if (is_string($value) && is_callable($value)).
@webmozart webmozart added Form Good first issue Ideal for your first contribution! (some Symfony experience may be required) DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Mar 3, 2016
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 5, 2016
fix symfony#17993 - Deprecated callable strings
8d45a0a
@Simperfit Simperfit mentioned this issue Mar 5, 2016
Merged
1 task
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 5, 2016
symfony#17993
c44ad21
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 5, 2016
symfony#17993
c087029
@Tobion
Copy link
Contributor

Tobion commented Mar 5, 2016

Exactly this was changed in #15561. To me, the behavior could be unexpected in both ways. But since people can just pass a PropertyPath instance directly if there is a conflict in name and the callable is the original contract, I thought it's better to change it to the current behavior.

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

Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 5, 2016
symfony#17993 - missing space after elseif / don't deprecate the whol…
96e2921 
…e method
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 5, 2016
symfony#17993 - add closure to avoid fatal errors
61707b8
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 5, 2016
symfony#17993 - Change message and adding empty line
9255f75
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 6, 2016
fix symfony#17993 - Deprecated callable strings
39e9859
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 6, 2016
fix symfony#17993 - Deprecated callable strings
b070941
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 7, 2016
fix symfony#17993 - Deprecated callable strings
4ae9bf6
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 7, 2016
fix symfony#17993 - Deprecated callable strings
dc6588e
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 7, 2016
fix symfony#17993 - Deprecated callable strings
edfe973
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 7, 2016
fix symfony#17993 - Deprecated callable strings
cd2bf8e
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 7, 2016
fix symfony#17993 - Deprecated callable strings
22f8264
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 7, 2016
fix symfony#17993 - Deprecated callable strings
5d49212
HeahDude added a commit to HeahDude/symfony that referenced this issue Mar 8, 2016
@HeahDude
[Form] fix fatal error handling callable strings with PropertyAccessD…
8c8c3ae 
…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 HeahDude mentioned this issue Mar 8, 2016
Closed
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 8, 2016
fix symfony#17993 - Deprecated callable strings
3ab86d5
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 8, 2016
fix symfony#17993 - Deprecated callable strings
96e3a42
@rodnaph
Copy link
Contributor

rodnaph commented Mar 10, 2016

I just ran into this as a regression when upgrading my Symfony version. In my case I was accessing a property called range on an EntityType field (which ended up calling the core range function as pointed out in the original description).

I'd agree this is confusing behaviour.

Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 12, 2016
fix symfony#17993 - Deprecated callable strings
b7f31ab
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 12, 2016
fix symfony#17993 - Deprecated callable strings
fc219d7
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 15, 2016
fix symfony#17993 - Deprecated callable strings
1733a49
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Mar 25, 2016
fix symfony#17993 - Deprecated callable strings
81a4a10
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 1, 2016
fix symfony#17993 - Deprecated callable strings
0b9e1db
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 1, 2016
fix symfony#17993 - Deprecated callable strings
edec798
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 1, 2016
fix symfony#17993 - Deprecated callable strings
c6625c9
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 1, 2016
fix symfony#17993 - Deprecated callable strings
4747e35
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 1, 2016
fix symfony#17993 - Deprecated callable strings
c3b996d
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 1, 2016
fix symfony#17993 - Deprecated callable strings
12975bd
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 1, 2016
fix symfony#17993 - Deprecated callable strings
4f2108f
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 2, 2016
fix symfony#17993 - Deprecated callable strings
a56a32b 
ext
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 5, 2016
fix symfony#17993 - Deprecated callable strings
d2f6434 
ext
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 5, 2016
fix symfony#17993 - Deprecated callable strings
b8aa179 
ext
Simperfit pushed a commit to Simperfit/symfony that referenced this issue Apr 6, 2016
fix symfony#17993 - Deprecated callable strings
560d229 
ext
@fabpot fabpot closed this as completed in 191b495 Apr 15, 2016
fabpot added a commit that referenced this issue Apr 15, 2016
@fabpot
feature #18020 fix #17993 - Deprecated callable strings (hamza)
47cb0c3 
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
@fabpot fabpot mentioned this issue May 13, 2016
Merged
@ogizanagi ogizanagi mentioned this issue May 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Form Good first issue Ideal for your first contribution! (some Symfony experience may be required)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@webmozart @rodnaph @Tobion