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

[WIP][2.4][Form] Add option "widget" to ChoiceType #8112

Closed
wants to merge 3 commits into from

Conversation

bamarni
Copy link
Contributor

@bamarni bamarni commented May 21, 2013

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? no
Fixed tickets #6602
License MIT
Doc PR not yet
  • add / fix tests / CS / PHPDocs
  • submit changes to the documentation
  • update php templates and twig table layout

I've implemented the behavior described in #6602.

Choice

  • Also, does choice_widget_select now fit better than choice_widget_collapsed?

Doctrine

  • For the original entity_id use case, it'd simply mean setting the property option to id.
<?php

$builder->add('field', 'entity', array(
    'class' => 'MyBundle:Product',
    'widget' => 'text',
    'property' => 'id',
));

Then the choice list won't be loaded in advance but validated on the fly.

What do you think about the approach? cc @bschussek


{# Deprecated in Symfony 2.4, to be removed in 3.0 #}

{% block choice_widget_collapsed %}{{ block('choice_widget_select') }}{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

This is a BC break: you are maintaining the compatibility for people calling {{ block('choice_widget_collapsed') }} but not for people extending choice_widget_collapsed in their custom theme (which is probably more common) as you never call this block .

@stof
Copy link
Member

stof commented May 21, 2013

As I said in the inline comment, using the label is wrong. Labels are not identifiers. You cannot require uniqueness on them.

@bamarni
Copy link
Contributor Author

bamarni commented May 21, 2013

@stof : good catches, I'll try to update the PR accordingly.

About "values or labels" discussion, clearly both of them are valid use cases to me, but as the original "entity_id" idea could easily be done with the current implementation, I went for labels. The opposite isn't true, for example in an autocompletion field, you'd have to maintain a hidden field containing ids, while you'd show an non-mapped field where autocompletion is applied and populated with labels.

What about an option then? view_data for example, that could be set to label or value.

Edit : see genemu/GenemuFormBundle#122, for the autocompletion use case.

@stof
Copy link
Member

stof commented May 22, 2013

@bamarni as I said previously, labels are not valid. You cannot identify the choice by its label. You can have several choices with the same label (or even all of them).

@bamarni
Copy link
Contributor Author

bamarni commented May 22, 2013

Actually my implementation is not the way to go, thoses 2 methods I've introduced don't make sense because here with the text widget there is no such things as "labels" anymore, we're only dealing with "values".

So the option I'm talking about would only replace values by labels in the ChoiceList object, and it wouldn't add any data transformer or whatsoever, its behavior would remain the same, of course as you said, when enabling this, uniqueness would obviously be required on labels.

A simple use case to legitimate this option, let's say I have a User entity. In my backoffice, I want to let admins search for a user by its id through a text input. On the other end, on the frontoffice, I have a simple user mailing page, users can enter a message, and then populate a text field with a recipient (user login).

@bamarni
Copy link
Contributor Author

bamarni commented May 25, 2013

Digging into the code again I've found a much better approach, there is a createValue() extension point in choice lists, what do you think?

@Koc Koc mentioned this pull request Jul 7, 2013
@Koc
Copy link
Contributor

Koc commented Jul 7, 2013

@bamarni Will it works with composite PKs?

ping @bschussek

@bamarni
Copy link
Contributor Author

bamarni commented Jul 7, 2013

@Koc : yes and no, it can work with this kind of entity but as the property option can only be set to a single column, in that situation there has to be another unique column you can rely on, usually the label should do the trick I guess.

@bamarni
Copy link
Contributor Author

bamarni commented Sep 10, 2013

@bschussek : what do you think of this implementation? should I continue or you have something else in mind?

@Koc
Copy link
Contributor

Koc commented Oct 7, 2014

ping @webmozart

@colinodell
Copy link
Contributor

Is this feature still being worked on?

@bamarni
Copy link
Contributor Author

bamarni commented Feb 11, 2015

Not since a while, I was still waiting for @webmozart feedback about the approach. Basically it does a bit more than the suggested "entity_id" form type would, in the sense that it also allow to use any other entity property (of course it has to be unique). I can continue this PR if we decide this approach is the way to go.

@webmozart
Copy link
Contributor

Hi @bamarni, thank you again for working on this issue! Sorry you had to wait so long for my feedback.

The ChoiceList implementation changed quite a bit in Symfony 2.7. Do you think you could rebase your work on the 2.8 branch?

@bamarni
Copy link
Contributor Author

bamarni commented Jun 21, 2015

@webmozart : thanks for the feedback, I've implemented the first part for the moment (on the Form component), cf. #15053

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