Skip to content

[Form] Deprecate passing false as $label to ChoiceListFactoryInterface #33074

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

Closed
wants to merge 1 commit into from

Conversation

vudaltsov
Copy link
Contributor

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

Replaces #32955 , see #32955 (comment)

@nicolas-grekas nicolas-grekas added this to the next milestone Aug 9, 2019
@vudaltsov vudaltsov force-pushed the deprecate-label-false branch from 9b7ca2b to 90aaa6c Compare August 9, 2019 09:14
@nicolas-grekas
Copy link
Member

Passing false looks fine to me. I'm not sure we should do this for the sake of adding type hints. Every deprecation adds to the migration process, we already have a lot of them. I'd prefer updating the docblock and mention that false is legal (and explain the purpose of it of course.)

@vudaltsov
Copy link
Contributor Author

Okay, I agree, this went too far :)
I will fix the typehint in this PR and will target 3.4, ok? (To avoid creating another one)

@Tobion
Copy link
Contributor

Tobion commented Aug 11, 2019

Passing false looks fine to me. I'm not sure we should do this for the sake of adding type hints. Every deprecation adds to the migration process, we already have a lot of them.

I don't think anyone will be affected by this. Poeple use the option and are unlikely to have changed the implementation. Otherwise I would also say it's not worth it. But currently there are two ways to do the same thing: passing false and passing a callable that returns false. And passing false directly violates the interface. So either we need to break BC by changing the interface or we need to do this here. So to me, this PR is the better solution.

@vudaltsov
Copy link
Contributor Author

Then let's finish this! @yceruto , I've just committed what you proposed. Did I get you right?

@vudaltsov
Copy link
Contributor Author

@yceruto , thanx, done. Also added tests to make sure the deprecation is triggered by default.

@vudaltsov vudaltsov force-pushed the deprecate-label-false branch from 3b43b43 to 79a3ee2 Compare August 12, 2019 07:12
@nicolas-grekas
Copy link
Member

This looks clumsy. I'm for updating the type annotation and declare it as callable|false|null, with proper runtime validation that throws a TypeError when appropriate.

@yceruto
Copy link
Member

yceruto commented Aug 12, 2019

This looks clumsy. I'm for updating the type annotation and declare it as callable|false|null, with proper runtime validation that throws a TypeError when appropriate.

Yes, I agree it could be the simpler variant.

nicolas-grekas added a commit that referenced this pull request Aug 13, 2019
…teView $label argument (vudaltsov)

This PR was merged into the 3.4 branch.

Discussion
----------

[Form] Add bool type to ChoiceListFactoryInterface::createView $label argument

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

Replaces #33074
Ping @nicolas-grekas , @Tobion , @yceruto .

Commits
-------

8f5d1ca Add false type to ChoiceListFactoryInterface::createView $label argument
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
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.

5 participants