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

Remove choices as values in 3.0 #6459

Merged
merged 1 commit into from
May 22, 2016
Merged

Remove choices as values in 3.0 #6459

merged 1 commit into from
May 22, 2016

Conversation

weaverryan
Copy link
Member

Q A
Doc fix? yes
New docs? no
Applies to 3.0+
Fixed tickets n/a

I think we must have just forgotten to remove this option!

@wouterj
Copy link
Member

wouterj commented Apr 12, 2016

Well, it still exists (otherwise it would be a BC break). Only the default value changed in 3.0. It is deprecated as of Symfony 3.1.

@@ -19,7 +19,6 @@ To use this field, you must specify *either* ``choices`` or ``choice_loader`` op
| | - `choice_name`_ |
| | - `choice_translation_domain`_ |
| | - `choice_value`_ |
| | - `choices_as_values`_ |
Copy link
Contributor

Choose a reason for hiding this comment

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

It is deprecated as of 3.1, not removed :)

@xabbuh
Copy link
Member

xabbuh commented Apr 14, 2016

Technically it is not deprecated in 3.0, but I agree with Ryan to remove it from the as it does not support setting it to anything else than true and will trigger an exception otherwise.

@HeahDude
Copy link
Contributor

closes #6474.

@xabbuh Shouldn't we just say that it's not used anymore and that setting false will throw an exception then mark it as deprecated in 3.1+ doc ?

@xabbuh
Copy link
Member

xabbuh commented Apr 14, 2016

@HeahDude IIRC we always removed the description of features in the version they got deprecated (but here it doesn't make sense to document it imo).

@HeahDude
Copy link
Contributor

There were many issues on the repo and and questions on stack, although it is recommended to upgrade to 2.8 before upgrading to 3.0, not everyone does that 😢. I vote (without hard feeling) to keep it documented at the strict minimum by removing the "before/after" stuff but keeping a notice for the legacy reason of the default value. What do you think ?

@weaverryan
Copy link
Member Author

Now that I've read the discussion here, I think we should keep the option, but change the description to describe that it's not used anymore, and was something that helped you upgrade. Then, if people see this option in their code and look in the docs for it, they'll see the description.

@xabbuh
Copy link
Member

xabbuh commented May 21, 2016

Okay, then let's keep the option and update the description instead.

@weaverryan weaverryan force-pushed the remove_choices_as_values branch from 0494fa2 to 51ee6f4 Compare May 22, 2016 18:11
Well, actually keeping a short description, so people kind find a description
of it if they see it in their project.
@weaverryan weaverryan force-pushed the remove_choices_as_values branch from 51ee6f4 to 3d4ae1e Compare May 22, 2016 18:13
@weaverryan weaverryan merged commit 3d4ae1e into 3.0 May 22, 2016
weaverryan added a commit that referenced this pull request May 22, 2016
This PR was merged into the 3.0 branch.

Discussion
----------

Remove choices as values in 3.0

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 3.0+
| Fixed tickets | n/a

I think we must have just forgotten to remove this option!

Commits
-------

3d4ae1e Removing all instances of choices_as_values in 3.0
@weaverryan weaverryan deleted the remove_choices_as_values branch May 22, 2016 18:16
@HeahDude
Copy link
Contributor

Nice! Thanks!

@HeahDude
Copy link
Contributor

HeahDude commented May 23, 2016

@weaverryan any idea why the status is "needs work" here ?

@wouterj
Copy link
Member

wouterj commented May 24, 2016

@HeahDude because we didn't update the label to "finished" before merging.

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.

4 participants