Skip to content
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

[Form] improve deprecation message for "empty_value" and "choice_list" options. #17119

Merged

Conversation

hhamon
Copy link
Contributor

@hhamon hhamon commented Dec 23, 2015

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

@stof
Copy link
Member

stof commented Dec 23, 2015

$this cannot be used in a closure in PHP 5.3 (and it would always be a ChoiceType class anyway, even if you were using the EntityType type, so this won't help much in 5.4+)

@hhamon
Copy link
Contributor Author

hhamon commented Dec 23, 2015

So __CLASS__ instead is better!

@hhamon hhamon force-pushed the choice-type-improve-deprecation-message branch from 71a1736 to ff26475 Compare December 23, 2015 14:43
@hhamon
Copy link
Contributor Author

hhamon commented Dec 23, 2015

Changed get_class() usage to __CLASS__.

@stof
Copy link
Member

stof commented Dec 23, 2015

Does __CLASS__ works fine in closures even in PHP 5.3 ?

and btw, using the type name rather than the type class would be better in 2.7. People are referencing it by name, not by class in 2.7.

@hhamon
Copy link
Contributor Author

hhamon commented Dec 23, 2015

That's a good point! I'm gonna try to add the type name using the getName() method.

@hhamon
Copy link
Contributor Author

hhamon commented Dec 23, 2015

This snippet works on PHP 5.3.29.

<?php

namespace Foo\Bar;

class A
{
    public function b($c)
    {
        $d = function () use ($c) {
          return strtoupper($c).' ('.__CLASS__.')';
        };

        return sprintf('Hello %s!', $d());
    }
}

$a = new A();
echo $a->b('Hugo');

It correctly outputs Hello HUGO (Foo\Bar\A)!.

@@ -294,7 +294,7 @@ public function configureOptions(OptionsResolver $resolver)

$choiceListNormalizer = function (Options $options, $choiceList) use ($choiceListFactory) {
if ($choiceList) {
@trigger_error('The "choice_list" option is deprecated since version 2.7 and will be removed in 3.0. Use "choice_loader" instead.', E_USER_DEPRECATED);
@trigger_error(sprintf('The "choice_list" option of a %s form type is deprecated since version 2.7 and will be removed in 3.0. Use "choice_loader" instead.', __CLASS__), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

I propose to be a bit shorter : The "choice_list" option of %s is deprecated...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep! Will do it later tonight ;)

@hhamon hhamon force-pushed the choice-type-improve-deprecation-message branch from ff26475 to 72453a7 Compare December 24, 2015 08:56
@hhamon
Copy link
Contributor Author

hhamon commented Dec 24, 2015

@nicolas-grekas @stof is it better now?

@hhamon hhamon force-pushed the choice-type-improve-deprecation-message branch 3 times, most recently from 8cd34fe to 88e13a0 Compare December 27, 2015 12:46
if ($choiceList) {
@trigger_error('The "choice_list" option is deprecated since version 2.7 and will be removed in 3.0. Use "choice_loader" instead.', E_USER_DEPRECATED);
@trigger_error(sprintf('The "choice_list" option of the "%s" form type (%s) is deprecated since version 2.7 and will be removed in 3.0. Use "choice_loader" instead.', $that->getName(), __CLASS__), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

since we know that getName is deprecated in 2.8, shouldn't we only tell about __CLASS__? At least that's what should be done in 2.8, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

@nicolas-grekas this PR targets Symfony 2.7. In 2.8, we should indeed change it to use the class name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When it's merged, I will open the other PR against the 2.8 branch.

@hhamon hhamon force-pushed the choice-type-improve-deprecation-message branch 2 times, most recently from 558293c to b58c0e5 Compare December 29, 2015 09:58
@hhamon hhamon force-pushed the choice-type-improve-deprecation-message branch from b58c0e5 to 7878536 Compare December 29, 2015 10:00
@nicolas-grekas
Copy link
Member

👍

1 similar comment
@xabbuh
Copy link
Member

xabbuh commented Dec 29, 2015

👍

@nicolas-grekas
Copy link
Member

Thank you @hhamon.

@nicolas-grekas nicolas-grekas merged commit 7878536 into symfony:2.7 Dec 29, 2015
nicolas-grekas added a commit that referenced this pull request Dec 29, 2015
…choice_list" options. (hhamon)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] improve deprecation message for "empty_value" and "choice_list" options.

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Commits
-------

7878536 [Form] improve deprecation messages for the "empty_value" and "choice_list" options in the ChoiceType class.
@hhamon hhamon deleted the choice-type-improve-deprecation-message branch December 29, 2015 16:18
This was referenced Jan 14, 2016
@fabpot fabpot mentioned this pull request Feb 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants