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

[2.2] [Form] Added form type "entity_identifier" (#1946) #1951

Closed
wants to merge 8 commits into from

Conversation

Gregwar
Copy link
Contributor

@Gregwar Gregwar commented Aug 12, 2011

(You can have a look at the issue #1946)

The "entity_id" type allow you to do things such:

        $builder->add('city', 'entity_id', array(
            'query_builder' => function($repository, $id) {
                return $repository->createQueryBuilder('c')
                    ->where('c.id = :id AND c.available = 1')
                    ->setParameter('id', $id);
            },  
            'class' => 'Gregwar\TestBundle\Entity\City',
            'required' => false,
            'hidden' => true
        )); 

So, with some JS logics and UI, you can fill the field directly using the entity id.

If you pass hidden => false, a text input will be rendered, which can allow an user to provide the id manually.

If you don't pass a query_builder closure, ->find() will be used.

@ericclemmons
Copy link
Contributor

Have you tried using a FormExtension to add a prototype option, where you can define hidden, text, integer, etc.? I ask since entity performs most of this work & translation already: the issue is more along the lines of how to render it.

@Gregwar
Copy link
Contributor Author

Gregwar commented Aug 12, 2011

No, the issue is not about the way it's rendered.

As mentionned in the issue #1946, the problem with the entity type is that it will always fetch all the records from the database and then act like a choice type.

What if you want your user to choose among thousands of cities ?

And, even if you don't have many entries in your database, do you think that's normal to fetch everything in PHP and then look if the id is part of the results ?

$meta = $this->em->getClassMetadata($this->class);

if (!$meta->getReflectionClass()->isInstance($data))
throw new TransformationFailedException('Invalid data, must be an instance of '.$this->class);
Copy link
Member

Choose a reason for hiding this comment

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

you need to add the curly braces here.

}

$identifierField = $meta->getSingleIdentifierFieldName();
$id = $meta->getReflectionProperty($identifierField)->getValue($data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Using Reflection here is actually a problem because we lose the ability of the Proxy to load the missing properties of the entity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I use "Form\Util\PropertyPath" to get the id value then ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I sent you a PR on your Bundle which is inspired of the original EntityToIdTransformer.
btw, I don't think it's a good idea to use the Form PropertyPath.

@Gregwar
Copy link
Contributor Author

Gregwar commented Aug 16, 2011

I added the support of "property" options which you can use to search entries using an alternative field instead of the real database id.

Example of use

Imagine you want to implement a simple "message" entity which users can send to others, you want to add a recipient field in which they can type the login of the other users, you simply do:

$builder->add('recipient', 'entity_id', array(
    'class' => 'Acme\DemoBundle\Entity\User',
    'property' => 'login',
    'hidden' => false
));

And entity_id will do all the job !

You can of course put autocompletion and everything you want for your UI, and still override the query_builder to fetch the entity, but using the property instead of the id to search it.

}

if (!$result) {
throw new TransformationFailedException('Can not find entity');
Copy link
Member

Choose a reason for hiding this comment

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

TransformationFailedException are turned into errors by the Form class. This is the way to add errors in transformers (see the doc of the interface)

@Gregwar
Copy link
Contributor Author

Gregwar commented Aug 21, 2011

I just updated my FormBundle repo if you want to use entity_id :
https://github.com/Gregwar/FormBundle

$options = array_replace($defaultOptions, $options);

if (null === $options['class']) {
throw new \RunTimeException('You must provide a class option for the entity_id field');
Copy link
Member

Choose a reason for hiding this comment

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

it should be a FormException instead

@phidah
Copy link
Contributor

phidah commented Aug 22, 2011

+1

@winzou
Copy link

winzou commented Aug 23, 2011

Massive +1 to this PR.

Shouldn't you rename it to entity_single or something like that ? Entity_id is obsolete since you added the property option.

@Gregwar
Copy link
Contributor Author

Gregwar commented Aug 23, 2011

@winzou, that's right, I'm still not sure about the name...

Note that If you read "entity identifer", you understand well what the field does in any case; it is an information that identifies an entity. Maybe entity_identifier ?

The problem with entity_single is that the entity type can also be used to choose a single entity so it would be a little confusing

Any feedbacks about that ?

@phidah
Copy link
Contributor

phidah commented Aug 23, 2011

entity_identifier sounds fine to me.

@j
Copy link

j commented Sep 5, 2011

So I'm needing something similar to this. My current use case uses elastic search and javascript autopopulate on an input field. Upon clicking, I of course set that id to the hidden field, but I also need to populate a select options with relational data to what was clicked... So I get the argument error on both the hidden type as well as the choice type.

I'd rather not have to render another hidden type for an onchange event of the relational choice type... so perhaps having the option to specify what kind of "empty" field you would like to render?

And a name like "empty_entity" makes more sense to me

@Gregwar
Copy link
Contributor Author

Gregwar commented Sep 6, 2011

@jstout24, I think that would imply moving the "text" and "hidden" form types services from FrameworkBundle to the DoctrineBundle, and I'm not sure that's a good idea because of code coupling concerns.

And I'm not sure I understand why do you want this to be empty ?

@j
Copy link

j commented Sep 6, 2011

@Gregwar, I don't quite understand. I just mean that most use cases would need just a hidden field, but what about if you're using javascript to populate a select.

ie:

There are Users > Zipcode (100,000+ records) > SexOffenders (about 20 per zip)

Users need a location in a zipcode database (very large, as you were saying.. an autocomplete box passing ID to a hidden entity type would suffice for this.)

But let's say after they enter their zipcode, I want to populate a regular ... The select should automatically bind to entity SexOffender.. whereas using this FormType, I'd have to also write an onchange event to the SexOffender select to input that id to the hidden field. Maybe it doesn't need to be within this type unless it was renamed to "empty_entity" or something..... This current implementation is for only <input type="hidden".... /> However, if 'choices' => array() could ensure that no data is selected: $builder->add('sexOffenders', 'entity', array( 'class' => 'Acme\DemoBundle\Entity\SexOffender', 'choices' => array() )); Or just create an empty option for the EntityType: $builder->add('sexOffenders', 'entity', array( 'class' => 'Acme\DemoBundle\Entity\SexOffender', 'empty' => true )); Also, another thing I wanted to do was have a hidden DateType but was limited by the documentation (I'm not sure if it's possible) Thoughts?

@j
Copy link

j commented Sep 11, 2011

What's the status of this PR?

@stof
Copy link
Member

stof commented May 15, 2012

@jstout24 it would need to be done the same way than the current entity type: an abstract class extended for each Doctrine project to change the name (we cannot register 2 types with the same name), the deps injected in it (once the ORM registry and once the ODM registry) and eventually some other method needing to be changed (to support query builders for instance)

@j
Copy link

j commented May 16, 2012

@stof yup, I like the sound of that! :p

Sent from my iPhone

On May 15, 2012, at 2:28 PM, Christophe Coevoet
reply@reply.github.com
wrote:

@jstout24 it would need to be done the same way than the current entity type: an abstract class extended for each Doctrine project to change the name (we cannot register 2 types with the same name), the deps injected in it (once the ORM registry and once the ODM registry) and eventually some other method needing to be changed (to support query builders for instance)


Reply to this email directly or view it on GitHub:
#1951 (comment)

@webmozart
Copy link
Contributor

I'd prefer putting this into ChoiceType. Basically it is the same as ChoiceType but with a different widget and lazy loading.

@j
Copy link

j commented May 16, 2012

@bschussek yeah it's a choice (kind of) if one was to have an input with some sort of javascript autocomplete. It's not much of a ChoiceType if it's not used in this manner.

For example, from a recent problem I ran into, I had a Form with a collection... I added javascript to reposition the items on the DOM and do an ajax post. After the first post, all is great. If I reposition without reloading the form and rebinding the data, I get crazy weird results. I'm assuming this is from the 0-indexed behaviour the ArrayCollection? This may be a problem with the collection type.

I used to do it like so:

<input type="hidden" name="children[0][id]" value="9" />
<input type="hidden" name="children[0][position]" value="1" />
<input type="hidden" name="children[1][id]" value="3" />
<input type="hidden" name="children[1][position]" value="2" />

I could open up an issue on the above if it makes sense to.

Anyway, back to this topic.

This is why I was in favor of being able to associate a hidden field to a model and make it easier for other ORMs/etc to adapt to.

If it's always going to be up to the method of setting data of the form to associate models, and the hidden entity type is always going to be for "choice" functionality ChoiceType makes more sense imo.

@thalionath
Copy link

+1 to this request

@jnonon
Copy link

jnonon commented Jun 27, 2012

Any updates?

@stof
Copy link
Member

stof commented Jun 27, 2012

As you could see in the title and the milestone, this has been scheduled for 2.2 as 2.1 is now frozen as it is in beta

@jnonon
Copy link

jnonon commented Jun 27, 2012

I did not noticed. Thanks for replying.

@lmcd
Copy link
Contributor

lmcd commented Oct 5, 2012

Any idea when this will land?

return null;
}
if (!$this->unitOfWork->isInIdentityMap($data)) {
throw new FormException('Entities passed to the choice field must be managed');
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the entity have to be managed if a property is provided? In my case, I'm providing an unserialized entity from the session that's not managed by the entity manager. It has an ID, so there's no reason for it not to work.

Can we move this check after if ($this->property) {}?

@mvrhov
Copy link

mvrhov commented Nov 2, 2012

I'm also wondering when this one will land

// Call the closure with the repository and the id
$qb = $qb($repository, $data);

try {

Choose a reason for hiding this comment

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

Why not use getOneOrNullResult?

@Gregwar
Copy link
Contributor Author

Gregwar commented Nov 22, 2012

Yes, this PR is going too far IMO, more than one year and very much comments... @fabpot, can you please do something about it?

@come
Copy link

come commented Nov 22, 2012

+1 to this request

@mkleins
Copy link

mkleins commented Nov 22, 2012

faster, stronger. I like this PR.

@luxifer
Copy link
Contributor

luxifer commented Nov 22, 2012

+1 for ths PR

@bamarni
Copy link
Contributor

bamarni commented Nov 23, 2012

Well this will probably not be merged as @bschussek stated he preferred having this done in entity type or maybe abstract it into choice type.

));
}

public function getDefaultOptions(array $options)
Copy link
Contributor

Choose a reason for hiding this comment

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

The method is deprecated.

Choose a reason for hiding this comment

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

You should have a look to https://github.com/Gregwar/FormBundle/blob/master/Type/EntityIdType.php#L40
The component is up to date there ! :)

@apsylone
Copy link

+1 for this PR ! What about it @fabpot, @stof ?!

@nurikabe
Copy link

nurikabe commented Dec 4, 2012

+1

1 similar comment
@joericochuyt
Copy link

+1

@michaelperrin
Copy link
Contributor

This form type could avoid the need to create data transformers in some cases, making code easier to read & write.

+1 !

@webmozart
Copy link
Contributor

Closed in favor of #6602.

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