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

fix #17993 - Deprecated callable strings #18020

Merged
merged 1 commit into from Apr 15, 2016

Conversation

Projects
None yet
10 participants
@Simperfit
Contributor

Simperfit commented Mar 5, 2016

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 ?

  • Rebase when #18057 is merged
@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Mar 5, 2016

Contributor

Thank you, I didn't understand the issue correctly.

I've changed my fix.

Contributor

Simperfit commented Mar 5, 2016

Thank you, I didn't understand the issue correctly.

I've changed my fix.

@javiereguiluz javiereguiluz added the Form label Mar 5, 2016

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Mar 5, 2016

Member

Not on these methods for sure.

Member

HeahDude commented Mar 5, 2016

Not on these methods for sure.

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Mar 5, 2016

Member

You could perhaps wrap the callable string in a closure inside those elseif to prevent the fatal error.

elseif (is_string($label) && is_callable($label)) {
    @trigger_error('createView() - $label - treats callable strings as callable and is deprecated since version 3.0.', E_USER_DEPRECATED);

    $label = function ($choice) use ($label) {
        return $label($choice);
    };
}
Member

HeahDude commented Mar 5, 2016

You could perhaps wrap the callable string in a closure inside those elseif to prevent the fatal error.

elseif (is_string($label) && is_callable($label)) {
    @trigger_error('createView() - $label - treats callable strings as callable and is deprecated since version 3.0.', E_USER_DEPRECATED);

    $label = function ($choice) use ($label) {
        return $label($choice);
    };
}
@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Mar 5, 2016

Contributor

I have no errors doing the tests except of one in PasswordEncoder.

Thanks @HeahDude for the help

Contributor

Simperfit commented Mar 5, 2016

I have no errors doing the tests except of one in PasswordEncoder.

Thanks @HeahDude for the help

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Mar 5, 2016

Member

I have no errors doing the tests except of one in PasswordEncoder.

What error are you talking about ?

Member

HeahDude commented Mar 5, 2016

I have no errors doing the tests except of one in PasswordEncoder.

What error are you talking about ?

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Mar 5, 2016

Contributor

Nevermind, no error anymore

Contributor

Simperfit commented Mar 5, 2016

Nevermind, no error anymore

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 6, 2016

Member

@Simperfit What about adding some unit tests?

Member

fabpot commented Mar 6, 2016

@Simperfit What about adding some unit tests?

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Mar 6, 2016

Contributor

@fabpot I added one, will add one for each method

Contributor

Simperfit commented Mar 6, 2016

@fabpot I added one, will add one for each method

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Mar 7, 2016

Member

Looks like this needs another rebase. It contains some unrelated changes.

Status: Needs work

Member

xabbuh commented Mar 7, 2016

Looks like this needs another rebase. It contains some unrelated changes.

Status: Needs work

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Mar 7, 2016

Contributor

@xabbuh Sorry for the unrelated changes, I've missed my rebase.

Contributor

Simperfit commented Mar 7, 2016

@xabbuh Sorry for the unrelated changes, I've missed my rebase.

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Mar 7, 2016

Contributor

@fabpot I've added one test for each method, is it ok like that ? Thank you.

Contributor

Simperfit commented Mar 7, 2016

@fabpot I've added one test for each method, is it ok like that ? Thank you.

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Mar 8, 2016

Member

Do we need to do all the stuff beside triggering the deprecation? Shouldn't the following code already handle the callback properly?

@xabbuh not when it is a callable string, see #17993.

Actually that's fine for $value (aka choice_value) as it gets the choice as only argument.

But all the others handled in createView() method (choice_name, choice_label, choice_attr, preferred_choices, group_by) would throw a fatal error because they get passed three arguments instead of one.

Consider @webmozart example:

$builder->add('deadline', ChoiceType::class, [
    'choices' => $dateRanges, // DateRange objects
    'choice_label' => 'end',
    // One would expect $choice->getEnd() to be called
    // but this will never happen
    // because PropertyAccessDecorator treats it as a callable
    // and not as a string so it does not transform it to a closure.
]);

Instead end($choice) is called, fine for value but fatal for the others.

So I suggest to say "Treating callable strings as callable is deprecated since version 3.1 and PropertyAccessDecorator will treat them as strings in 4.0. You should use a "\Closure" instead.".

To avoid the fatal error for now, we should wrap the callable string in a closure:

$label = function ($choice) use ($label) {
    return $label($choice);
}

So we get call_user_func($label, $choice, $key, $value) working until we can refactor in 4.0:

if (is_string($value) && !is_callable($value)) {
    $value = new PropertyPath($value);
} elseif (is_string($value) && is_callable($value)) {
   // Trigger, handle...
}

// To simply

if (is_string($value)) {
    $value = new PropertyPath($value);
}
Member

HeahDude commented Mar 8, 2016

Do we need to do all the stuff beside triggering the deprecation? Shouldn't the following code already handle the callback properly?

@xabbuh not when it is a callable string, see #17993.

Actually that's fine for $value (aka choice_value) as it gets the choice as only argument.

But all the others handled in createView() method (choice_name, choice_label, choice_attr, preferred_choices, group_by) would throw a fatal error because they get passed three arguments instead of one.

Consider @webmozart example:

$builder->add('deadline', ChoiceType::class, [
    'choices' => $dateRanges, // DateRange objects
    'choice_label' => 'end',
    // One would expect $choice->getEnd() to be called
    // but this will never happen
    // because PropertyAccessDecorator treats it as a callable
    // and not as a string so it does not transform it to a closure.
]);

Instead end($choice) is called, fine for value but fatal for the others.

So I suggest to say "Treating callable strings as callable is deprecated since version 3.1 and PropertyAccessDecorator will treat them as strings in 4.0. You should use a "\Closure" instead.".

To avoid the fatal error for now, we should wrap the callable string in a closure:

$label = function ($choice) use ($label) {
    return $label($choice);
}

So we get call_user_func($label, $choice, $key, $value) working until we can refactor in 4.0:

if (is_string($value) && !is_callable($value)) {
    $value = new PropertyPath($value);
} elseif (is_string($value) && is_callable($value)) {
   // Trigger, handle...
}

// To simply

if (is_string($value)) {
    $value = new PropertyPath($value);
}
@webmozart

This comment has been minimized.

Show comment
Hide comment
@webmozart

webmozart Mar 8, 2016

Contributor

Thanks for working on this @Simperfit! :)

Contributor

webmozart commented Mar 8, 2016

Thanks for working on this @Simperfit! :)

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Mar 8, 2016

Member

@HeahDude But if we solve it this way it means that we make a feature working while deprecating it at the same moment. If we think that we should wrap strings that are callables in a closure for safety, we should fix that issue in all affected branches (I guess 2.7 is the lowest affected branch, right?).

Member

xabbuh commented Mar 8, 2016

@HeahDude But if we solve it this way it means that we make a feature working while deprecating it at the same moment. If we think that we should wrap strings that are callables in a closure for safety, we should fix that issue in all affected branches (I guess 2.7 is the lowest affected branch, right?).

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Mar 8, 2016

Member

@xabbuh yes and no. The new feature would be:

if (is_string($value) && !is_callable($value)) {
    $value = new PropertyPath($value);
} elseif (is_string($value) && is_callable($value)) {
    // Trigger
    $value = new PropertyPath($value);
}

But your right my actual patch should go on 2.7, so I send a PR and this one will need a rebase though.

Member

HeahDude commented Mar 8, 2016

@xabbuh yes and no. The new feature would be:

if (is_string($value) && !is_callable($value)) {
    $value = new PropertyPath($value);
} elseif (is_string($value) && is_callable($value)) {
    // Trigger
    $value = new PropertyPath($value);
}

But your right my actual patch should go on 2.7, so I send a PR and this one will need a rebase though.

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Mar 8, 2016

Contributor

@webmozart thanks to you for helping me :)

@HeahDude I was looking into the PR you made but I can't make the test working, using both closure or PropertyPath, do you know what I'm missing ? Thank you for helping me from the start.

Contributor

Simperfit commented Mar 8, 2016

@webmozart thanks to you for helping me :)

@HeahDude I was looking into the PR you made but I can't make the test working, using both closure or PropertyPath, do you know what I'm missing ? Thank you for helping me from the start.

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Mar 31, 2016

Member

If I'm not wrong, as I've just learned with another PR, you should modify the CHANGELOG file of the Form component to just note the deprecation in 3.1 section:

  * Using callable strings as choice options in `ChoiceType` has been deprecated and
    will be used as `PropertyPath` instead of callable in Symfony 4.0.

And a note in the root UPGRADE 4.0 to explain:

  * Using callable strings as choice options in ChoiceType has been deprecated
     in favor of `PropertyPath`. Use a "\Closure" instead.

      Before:

      'choice_value' => new PropertyPath('range'),
      'choice_label' => 'strtoupper',

      After:

      'choice_value' => 'range',
      'choice_label' => function ($choice) {
          return strtoupper($choice);
      },
Member

HeahDude commented Mar 31, 2016

If I'm not wrong, as I've just learned with another PR, you should modify the CHANGELOG file of the Form component to just note the deprecation in 3.1 section:

  * Using callable strings as choice options in `ChoiceType` has been deprecated and
    will be used as `PropertyPath` instead of callable in Symfony 4.0.

And a note in the root UPGRADE 4.0 to explain:

  * Using callable strings as choice options in ChoiceType has been deprecated
     in favor of `PropertyPath`. Use a "\Closure" instead.

      Before:

      'choice_value' => new PropertyPath('range'),
      'choice_label' => 'strtoupper',

      After:

      'choice_value' => 'range',
      'choice_label' => function ($choice) {
          return strtoupper($choice);
      },
@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Mar 31, 2016

Contributor

Got it, thank you will do that @HeahDude 👍

Contributor

Simperfit commented Mar 31, 2016

Got it, thank you will do that @HeahDude 👍

@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Apr 1, 2016

Member

@Simperfit I've edited my comment, please use this new version instead. Thanks :)

Member

HeahDude commented Apr 1, 2016

@Simperfit I've edited my comment, please use this new version instead. Thanks :)

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Apr 1, 2016

Contributor

@HeahDude I've added them

Contributor

Simperfit commented Apr 1, 2016

@HeahDude I've added them

Show outdated Hide outdated UPGRADE-4.0.md
Show outdated Hide outdated UPGRADE-4.0.md
@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Apr 1, 2016

Member

Minor comments, then you'll need to rebase on current master because there is now some conflicts.

Thanks

Member

HeahDude commented Apr 1, 2016

Minor comments, then you'll need to rebase on current master because there is now some conflicts.

Thanks

Show outdated Hide outdated UPGRADE-4.0.md
Show outdated Hide outdated UPGRADE-4.0.md
Show outdated Hide outdated UPGRADE-4.0.md
Show outdated Hide outdated UPGRADE-4.0.md
@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Apr 1, 2016

Contributor

I've rebased and added/removed the space :p

Contributor

Simperfit commented Apr 1, 2016

I've rebased and added/removed the space :p

Show outdated Hide outdated UPGRADE-4.0.md
Show outdated Hide outdated UPGRADE-4.0.md
@HeahDude

This comment has been minimized.

Show comment
Hide comment
@HeahDude

HeahDude Apr 2, 2016

Member

Looks perfect now :) Thanks @Simperfit!

This one is ready to merge now.

Deprecate or revert 470b140 ? ping @symfony/decoders

Member

HeahDude commented Apr 2, 2016

Looks perfect now :) Thanks @Simperfit!

This one is ready to merge now.

Deprecate or revert 470b140 ? ping @symfony/decoders

@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit
Contributor

Simperfit commented Apr 2, 2016

Show outdated Hide outdated UPGRADE-4.0.md
Show outdated Hide outdated UPGRADE-4.0.md
hamza
@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Apr 15, 2016

Contributor

i've rebased with master.
ping @fabpot @webmozart @Tobion

Contributor

Simperfit commented Apr 15, 2016

i've rebased with master.
ping @fabpot @webmozart @Tobion

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Apr 15, 2016

Member

IIRC, we were waiting for @Tobion's opinion about merging or reverting instead.

Member

fabpot commented Apr 15, 2016

IIRC, we were waiting for @Tobion's opinion about merging or reverting instead.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Apr 15, 2016

Member

I would revert but I'm ok with both ways.

Member

Tobion commented Apr 15, 2016

I would revert but I'm ok with both ways.

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Apr 15, 2016

Member

Thank you @Simperfit.

Member

fabpot commented Apr 15, 2016

Thank you @Simperfit.

@fabpot fabpot merged commit 191b495 into symfony:master Apr 15, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 15, 2016

feature #18020 fix #17993 - Deprecated callable strings (hamza)
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
@Simperfit

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Apr 17, 2016

Contributor

NP @fabpot.

Contributor

Simperfit commented Apr 17, 2016

NP @fabpot.

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