8000 [2.2] [Form] Added form type "entity_identifier" (#1946) by Gregwar · Pull Request #1951 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

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

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 8 commits into from
Next Next commit
[Form] Added the entity_id type (#1946)
  • Loading branch information
Gregwar committed Aug 12, 2011
commit 05a064a93ba0029357d734ba147fca22e11f8921
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bridge\Doctrine\Form\DataTransformer;

use Symfony\Component\Form\FormInterface;
use Symfony\Component\Form\DataTransformerInterface;
use Symfony\Component\Form\FormError;
use Symfony\Component\Form\Exception\TransformationFailedException;
use Symfony\Component\Form\Exception\UnexpectedTypeException;

use Doctrine\ORM\EntityManager;
use Doctrine\ORM\NoResultException;

class OneEntityToIdTransformer implements DataTransformerInterface
{
private $em;
private $class;
private $queryBuilder;

public function __construct(EntityManager $em, $class, $queryBuilder)
{
if (!(null === $queryBuilder || $queryBuilder instanceof \Closure)) {
Copy link
Member

Choose a reason for hiding this comment

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

you should write it if (null !== $queryBuilder && ! $queryBuilder instanceof \Closure) instead.

Also, what about allowing any callable instead of forcing to use a closure ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is almost the same as in "../ChoiceList/EntityChoiceList.php" which is written so and just accepts closure, I must confess I don't know what it would not be any callable

throw new UnexpectedTypeException($queryBuilder, 'Doctrine\ORM\QueryBuilder or \Closure');
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 wrong. The check forbids giving a QueryBuilder instance so the error message should not say it is allowed

}

if (null == $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 should use ===

throw new UnexpectedTypeException($class, 'string');
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 curly braces


$this->em = $em;
$this->class = $class;
$this->queryBuilder = $queryBuilder;
}

/**
* Fetch the id of the entity to populate the form
*/
public function transform($data)
{
if (null === $data)
return null;
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 curly braces here too


$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.


return $id;
}

/**
* Try to fetch the entity from its id in the database
*/
public function reverseTransform($data)
{
if (!$data) {
return null;
}

$em = $this->em;
$class = $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.

why doing this ?

$repository = $em->getRepository($class);

if ($qb = $this->queryBuilder) {
// If a closure was passed, call id with the repository and the id
if ($qb instanceof \Closure) {
$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?

$result = $qb->getQuery()->getSingleResult();
Copy link
Member

Choose a reason for hiding this comment

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

this does not make sense in the case where a QueryBuilder is passed directly as it does not receive the data at all

Copy link
Member

Choose a reason for hiding this comment

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

ah sorry, looking at the constructor, the queryBuilder property is always a closure (the naming is bad then) which means that the previous check is useless and that a check is missing to be sure you receive a query builder

} catch (NoResultException $e) {
$result = null;
}
} else {
// Defaults to find()
$result = $repository->find($data);
}

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.

curly braces are missing here too

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)


return $result;
}
}

59 changes: 59 additions & 0 deletions src/Symfony/Bridge/Doctrine/Form/Type/EntityIdType.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bridge\Doctrine\Form\Type;

use Symfony\Component\Form\FormBuilder;
use Symfony\Bridge\Doctrine\RegistryInterface;
use Symfony\Bridge\Doctrine\Form\DataTransformer\OneEntityToIdTransformer;
use Symfony\Component\Form\AbstractType;

class EntityIdType extends AbstractType
{
protected $em;

public function __construct(RegistryInterface $registry)
{
$this->em = $registry->getEntityManager();
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 wrong. You should keep the reference to the registry instead and then retrieve the good entity manager according to the name passed as option, like in the EntityType.

}

public function buildForm(FormBuilder $builder, array $options)
{
$em = $options['em'] ?: $this->em;

$builder->prependClientTransformer(new OneEntityToIdTransformer($em, $options['class'], $options['query_builder']));
}

public function getDefaultOptions(array $options)
{
$defaultOptions = array(
'required' => true,
'em' => null,
'class' => null,
'query_builder' => null,
'hidden' => true
);

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

return $defaultOptions;
}

public function getParent(array $options)
{
return $options['hidden'] ? 'hidden' : 'field';
}

public function getName()
{
return 'entity_id';
}
}
5 changes: 5 additions & 0 deletions src/Symfony/Bundle/DoctrineBundle/Resources/config/orm.xml
7E91
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@
<argument type="service" id="doctrine" />
</service>

<service id="form.type.entity_id" class="Symfony\Bridge\Doctrine\Form\Type\EntityIdType">
<tag name="form.type" alias="entity_id" />
<argument type="service" id="doctrine" />
</service>

<service id="doctrine.orm.configuration" class="%doctrine.orm.configuration.class%" abstract="true" public="false" />

<service id="doctrine.orm.entity_manager.abstract" class="%doctrine.orm.entity_manager.class%" factory-class="%doctrine.orm.entity_manager.class%" factory-method="create" abstract="true" />
Expand Down
0