NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

fix #17993 - Deprecated callable strings#18020

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromSimperfit:master

Apr 15, 2016

Merged

fix #17993 - Deprecated callable strings#18020

fabpot merged 1 commit intosymfony:masterfromSimperfit:master

Apr 15, 2016

Conversation

Copy link

Contributor

Simperfit commentedMar 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[Form] handle callable strings in PropertyAccessDecorator #18057 is merged

carsonbot added the Status: Needs Review label

Mar 5, 2016

Copy link

ContributorAuthor

Simperfit commentedMar 5, 2016

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

I've changed my fix.

javiereguiluz added the Form label

Mar 5, 2016

Tobion reviewed

Mar 5, 2016

View reviewed changes

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


		if (is_string($preferredChoices) && !is_callable($preferredChoices)) {
		$preferredChoices =newPropertyPath($preferredChoices);
		}elseif(is_string($preferredChoices) &&is_callable($preferredChoices)) {

Copy link

Contributor

Choose a reason for hiding this comment

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

missing space after elseif

Copy link

Contributor

Choose a reason for hiding this comment

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

since version 3.1 (for now)

Copy link

Contributor

Simperfit commentedMar 5, 2016

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

Thanks@HeahDude for the help

linaori reviewed

Mar 5, 2016

View reviewed changes

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

		if (is_string($value) && !is_callable($value)) {
		$value =newPropertyPath($value);
		}elseif (is_string($value) &&is_callable($value)) {
		@trigger_error('createListFromChoices() treats callable strings as callable and is deprecated since version 3.1.',E_USER_DEPRECATED);

Copy link

Contributor

Choose a reason for hiding this comment

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

If it's deprecated, what's the recommended thing to do instead? What will happen in 4.0? Please add this to the deprecation messages (and upgrade guide)

Copy link

Contributor

Choose a reason for hiding this comment

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

@iltar is right. You should change each message to something like :

'Treating strings as callable is deprecated since version 3.1 and will throw an error in 4.0. You should use a "\Closure" instead.'

Copy link

Contributor

HeahDude commentedMar 5, 2016

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

What error are you talking about ?

Copy link

ContributorAuthor

Simperfit commentedMar 5, 2016

Nevermind, no error anymore

Copy link

Member

fabpot commentedMar 6, 2016

@Simperfit What about adding some unit tests?

Simperfit force-pushed themaster branch from9255f75 to39e9859Compare

March 6, 2016 14:51

javiereguiluz added the Deprecation label

Mar 6, 2016

Simperfit force-pushed themaster branch from35c2c52 tob070941Compare

March 6, 2016 22:48

Copy link

ContributorAuthor

Simperfit commentedMar 6, 2016

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

Copy link

Member

xabbuh commentedMar 7, 2016

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

Status: Needs work

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Mar 7, 2016

Simperfit force-pushed themaster branch fromb070941 to4ae9bf6Compare

March 7, 2016 18:18

Copy link

ContributorAuthor

Simperfit commentedMar 7, 2016

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

xabbuh reviewed

Mar 7, 2016

View reviewed changes

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

		return$value($choices);
		};

		return$value($choices);

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I've followed the instruction of someone since I don't wanted to make bad things,
But you're right, adding the test without any code is the right thing to do.

Thanks

Simperfit force-pushed themaster branch 4 times, most recently fromcd2bf8e to22f8264Compare

March 7, 2016 19:20

Copy link

ContributorAuthor

Simperfit commentedMar 7, 2016

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

Simperfit force-pushed themaster branch fromedec798 toc6625c9Compare

April 1, 2016 18:48

HeahDude reviewed

Apr 1, 2016

View reviewed changes

UPGRADE-4.0.md Outdated

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

		Before:

Copy link

Contributor

Choose a reason for hiding this comment

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

Ok now you got one extra here

Simperfit force-pushed themaster branch fromc6625c9 to4747e35Compare

April 1, 2016 18:55

Copy link

ContributorAuthor

Simperfit commentedApr 1, 2016

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

Simperfit force-pushed themaster branch 2 times, most recently fromc3b996d to12975bdCompare

April 1, 2016 19:04

HeahDude reviewed

Apr 1, 2016

View reviewed changes

UPGRADE-4.0.md Outdated

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

Copy link

Contributor

Choose a reason for hiding this comment

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

You misunderstood the "four" space here mean four for this line only.

Now on got one extra space from line 23 to 37 and missing 3 here one 35 :p

Sorry, after that we're done, thank you for working on this PR :)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I hope this time it's ok :p

Np :)

Simperfit force-pushed themaster branch from12975bd to4f2108fCompare

April 1, 2016 19:22

HeahDude reviewed

Apr 1, 2016

View reviewed changes

UPGRADE-4.0.md Outdated

		-`"form.type.submit"`
		-`"form.type.reset"`

		`ArrayAccess` in`ResizeFormListener::preSubmit` method has been removed

Copy link

Contributor

Choose a reason for hiding this comment

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

~~to not conflict with master it must be before your change~~

ok just remove it

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

oops sorry

Simperfit force-pushed themaster branch from4f2108f toa56a32bCompare

April 2, 2016 08:15

Copy link

Contributor

HeahDude commentedApr 2, 2016

Looks perfect now :) Thanks@Simperfit!

This one is ready to merge now.

Deprecate or revert470b140 ? ping@symfony/decoders

Copy link

ContributorAuthor

Simperfit commentedApr 2, 2016

ping@Tobion @fabpot @dunglas

xabbuh reviewed

Apr 5, 2016

View reviewed changes

UPGRADE-4.0.md Outdated

		`ArrayAccess` in`ResizeFormListener::preSubmit` method has been removed.

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

Copy link

Member

Choose a reason for hiding this comment

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

The sentence as is must be added to theUPGRADE-3.1.md file. In this file it should read like "Using callable strings as choice options in ChoiceType has been removed in favor of passingPropertyPath instances."

Copy link

Contributor

HeahDudeApr 5, 2016

Choose a reason for hiding this comment

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

👍

Copy link

ContributorAuthor

SimperfitApr 5, 2016

Choose a reason for hiding this comment

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

is it ok like this@HeahDude @xabbuh ?

Simperfit force-pushed themaster branch froma56a32b tod2f6434Compare

April 5, 2016 17:33

HeahDude reviewed

Apr 5, 2016

View reviewed changes

UPGRADE-4.0.md

		`ArrayAccess` in`ResizeFormListener::preSubmit` method has been removed.

		* Using callable strings as choice options in ChoiceType has been removed in favor of passing PropertyPath instances.

Copy link

Contributor

HeahDudeApr 5, 2016

Choose a reason for hiding this comment

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

I would replace "has been removed" by "is not supported anymore".

But please wait for a symfony decider to tell you if it's ok. Thanks :)

Copy link

Member

xabbuhApr 5, 2016

Choose a reason for hiding this comment

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

You are right. That sounds a bit better.

Copy link

ContributorAuthor

SimperfitApr 5, 2016

Choose a reason for hiding this comment

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

done :)

Simperfit force-pushed themaster branch fromd2f6434 tob8aa179Compare

April 5, 2016 18:03

fabpot reviewed

Apr 6, 2016

View reviewed changes

src/Symfony/Component/Form/CHANGELOG.md Outdated

		`ArrayAccess` in`ResizeFormListener::preSubmit` method

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

Copy link

Member

fabpotApr 6, 2016

Choose a reason for hiding this comment

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

missing space at the beginning of the line

Copy link

ContributorAuthor

SimperfitApr 6, 2016

Choose a reason for hiding this comment

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

@fabpot space added.

Simperfit force-pushed themaster branch fromb8aa179 to560d229Compare

April 6, 2016 21:10

fixsymfony#17993- Deprecated callable strings

191b495

ext

Simperfit force-pushed themaster branch from560d229 to191b495Compare

April 15, 2016 05:56

Copy link

ContributorAuthor

Simperfit commentedApr 15, 2016

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

Copy link

Member

fabpot commentedApr 15, 2016

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

Copy link

Contributor

Tobion commentedApr 15, 2016

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

Copy link

Member

fabpot commentedApr 15, 2016

Thank you@Simperfit.

fabpot merged commit191b495 intosymfony:master

Apr 15, 2016

fabpot added a commit that referenced this pull request

Apr 15, 2016

feature#18020fix#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 mergedCommits-------191b495fix#17993 - Deprecated callable strings