Skip to content

Commit

Permalink
merged branch jmikola/2.0-LoaderResolverInterface (PR #2785)
Browse files Browse the repository at this point in the history
Commits
-------

7c1cbb9 [Config] Use LoaderResolverInterface for type-hinting
48b084e fixed typo
8ad94fb merged branch hhamon/doctrine_bridge_cs (PR #2775)
240796e [Bridge] [Doctrine] fixed coding conventions.
7cfc392 check for session before trying to authentication details
648fae7 merged branch proofek/domcrawlerform-radiodisabled (PR #2768)
3976b7a [DoctrineBridge] fixed CS
9a04783 merged branch beberlei/SecurityEntityRepositoryIdentifierFix (PR #2765)
3c83b89 [DoctrineBridge] Catch user-error when the identifier is not serialized with the User entity.
36c7d03 Fixed GH-2720 - Fix disabled atrribute handling for radio form elements

Discussion
----------

[Config] Use LoaderResolverInterface for type-hinting

```
Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: -
```

I've listed this as a BC break because we're changing the argument type-hint, but I think it's unlikely to affect anyone.
  • Loading branch information
fabpot committed Dec 5, 2011
2 parents 6924e63 + 7c1cbb9 commit 3026287
Show file tree
Hide file tree
Showing 10 changed files with 147 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ private function loadEntities($entities, $group = null)
throw new FormException('Entities passed to the choice field must have a "__toString()" method defined (or you can also override the "property" option).');
}

$value = (string)$entity;
$value = (string) $entity;
}

if (count($this->identifier) > 1) {
Expand Down
11 changes: 10 additions & 1 deletion src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,21 @@ public function refreshUser(UserInterface $user)
if (!$user instanceof $this->class) {
throw new UnsupportedUserException(sprintf('Instances of "%s" are not supported.', get_class($user)));
}


// The user must be reloaded via the primary key as all other data
// might have changed without proper persistence in the database.
// That's the case when the user has been changed by a form with
// validation errors.
return $this->repository->find($this->metadata->getIdentifierValues($user));
if (!$id = $this->metadata->getIdentifierValues($user)) {
throw new \InvalidArgumentException("You cannot refresh a user ".
"from the EntityUserProvider that does not contain an identifier. ".
"The user object has to be serialized with its own identifier " .
"mapped by Doctrine."
);
}

return $this->repository->find($id);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ public function isValid($entity, Constraint $constraint)
throw new UnexpectedTypeException($constraint->fields, 'array');
}

$fields = (array)$constraint->fields;
$fields = (array) $constraint->fields;

if (count($fields) == 0) {
throw new ConstraintDefinitionException("At least one field has to be specified.");
if (0 === count($fields)) {
throw new ConstraintDefinitionException('At least one field has to be specified.');
}

if ($constraint->em) {
Expand All @@ -66,22 +66,21 @@ public function isValid($entity, Constraint $constraint)
$criteria = array();
foreach ($fields as $fieldName) {
if (!isset($class->reflFields[$fieldName])) {
throw new ConstraintDefinitionException("Only field names mapped by Doctrine can be validated for uniqueness.");
throw new ConstraintDefinitionException('Only field names mapped by Doctrine can be validated for uniqueness.');
}

$criteria[$fieldName] = $class->reflFields[$fieldName]->getValue($entity);

if ($criteria[$fieldName] === null) {
if (null === $criteria[$fieldName]) {
return true;
} else if (isset($class->associationMappings[$fieldName])) {
}

if (isset($class->associationMappings[$fieldName])) {
$relatedClass = $em->getClassMetadata($class->associationMappings[$fieldName]['targetEntity']);
$relatedId = $relatedClass->getIdentifierValues($criteria[$fieldName]);

if (count($relatedId) > 1) {
throw new ConstraintDefinitionException(
"Associated entities are not allowed to have more than one identifier field to be " .
"part of a unique constraint in: " . $class->name . "#" . $fieldName
);
throw new ConstraintDefinitionException(sprintf('Associated entities are not allowed to have more than one identifier field to be part of a unique constraint in %s#%s.', $class->name, $fieldName));
}
$criteria[$fieldName] = array_pop($relatedId);
}
Expand All @@ -94,12 +93,12 @@ public function isValid($entity, Constraint $constraint)
* which is the same as the entity being validated, the criteria is
* unique.
*/
if (0 == count($result) || (1 == count($result) && $entity === $result[0])) {
if (0 === count($result) || (1 === count($result) && $entity === $result[0])) {
return true;
}

$oldPath = $this->context->getPropertyPath();
$this->context->setPropertyPath( empty($oldPath) ? $fields[0] : $oldPath.".".$fields[0]);
$this->context->setPropertyPath( empty($oldPath) ? $fields[0] : $oldPath.'.'.$fields[0]);
$this->context->addViolation($constraint->message, array(), $criteria[$fields[0]]);
$this->context->setPropertyPath($oldPath);

Expand Down
6 changes: 3 additions & 3 deletions src/Symfony/Component/Config/Loader/Loader.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ abstract class Loader implements LoaderInterface
/**
* Gets the loader resolver.
*
* @return LoaderResolver A LoaderResolver instance
* @return LoaderResolverInterface A LoaderResolverInterface instance
*/
public function getResolver()
{
Expand All @@ -35,9 +35,9 @@ public function getResolver()
/**
* Sets the loader resolver.
*
* @param LoaderResolver $resolver A LoaderResolver instance
* @param LoaderResolverInterface $resolver A LoaderResolverInterface instance
*/
public function setResolver(LoaderResolver $resolver)
public function setResolver(LoaderResolverInterface $resolver)
{
$this->resolver = $resolver;
}
Expand Down
6 changes: 3 additions & 3 deletions src/Symfony/Component/Config/Loader/LoaderInterface.php
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ function supports($resource, $type = null);
/**
* Gets the loader resolver.
*
* @return LoaderResolver A LoaderResolver instance
* @return LoaderResolverInterface A LoaderResolverInterface instance
*/
function getResolver();

/**
* Sets the loader resolver.
*
* @param LoaderResolver $resolver A LoaderResolver instance
* @param LoaderResolverInterface $resolver A LoaderResolverInterface instance
*/
function setResolver(LoaderResolver $resolver);
function setResolver(LoaderResolverInterface $resolver);

}
91 changes: 81 additions & 10 deletions src/Symfony/Component/DomCrawler/Field/ChoiceFormField.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,22 @@ public function hasValue()
return true;
}

/**
* Check if the current selected option is disabled
*
* @return bool
*/
public function isDisabled()
{
foreach ($this->options as $option) {
if ($option['value'] == $this->value && $option['disabled']) {
return true;
}
}

return false;
}

/**
* Sets the value of the field.
*
Expand Down Expand Up @@ -101,20 +117,20 @@ public function setValue($value)
$this->value = null;
} elseif ('checkbox' == $this->type && true === $value) {
// check
$this->value = $this->options[0];
$this->value = $this->options[0]['value'];
} else {
if (is_array($value)) {
if (!$this->multiple) {
throw new \InvalidArgumentException(sprintf('The value for "%s" cannot be an array.', $this->name));
}

foreach ($value as $v) {
if (!in_array($v, $this->options)) {
throw new \InvalidArgumentException(sprintf('Input "%s" cannot take "%s" as a value (possible values: %s).', $this->name, $v, implode(', ', $this->options)));
if (!$this->containsOption($v, $this->options)) {
throw new \InvalidArgumentException(sprintf('Input "%s" cannot take "%s" as a value (possible values: %s).', $this->name, $v, implode(', ', $this->availableOptionValues())));
}
}
} elseif (!in_array($value, $this->options)) {
throw new \InvalidArgumentException(sprintf('Input "%s" cannot take "%s" as a value (possible values: %s).', $this->name, $value, implode(', ', $this->options)));
} elseif (!$this->containsOption($value, $this->options)) {
throw new \InvalidArgumentException(sprintf('Input "%s" cannot take "%s" as a value (possible values: %s).', $this->name, $value, implode(', ', $this->availableOptionValues())));
}

if ($this->multiple) {
Expand Down Expand Up @@ -144,10 +160,11 @@ public function addChoice(\DOMNode $node)
throw new \LogicException(sprintf('Unable to add a choice for "%s" as it is not multiple or is not a radio button.', $this->name));
}

$this->options[] = $value = $node->hasAttribute('value') ? $node->getAttribute('value') : '1';
$option = $this->buildOptionValue($node);
$this->options[] = $option;

if ($node->getAttribute('checked')) {
$this->value = $value;
$this->value = $option['value'];
}
}

Expand Down Expand Up @@ -192,10 +209,11 @@ protected function initialize()

if ('input' == $this->node->nodeName) {
$this->type = $this->node->getAttribute('type');
$this->options[] = $value = $this->node->hasAttribute('value') ? $this->node->getAttribute('value') : '1';
$optionValue = $this->buildOptionValue($this->node);
$this->options[] = $optionValue;

if ($this->node->getAttribute('checked')) {
$this->value = $value;
$this->value = $optionValue['value'];
}
} else {
$this->type = 'select';
Expand All @@ -207,7 +225,7 @@ protected function initialize()

$found = false;
foreach ($this->xpath->query('descendant::option', $this->node) as $option) {
$this->options[] = $option->hasAttribute('value') ? $option->getAttribute('value') : $option->nodeValue;
$this->options[] = $this->buildOptionValue($option);

if ($option->getAttribute('selected')) {
$found = true;
Expand All @@ -226,4 +244,57 @@ protected function initialize()
}
}
}

/**
* Returns option value with associated disabled flag
*
* @param type $node
*
* @return array
*/
private function buildOptionValue($node)
{
$option = array();

$defaultValue = (isset($node->nodeValue) && !empty($node->nodeValue)) ? $node->nodeValue : '1';
$option['value'] = $node->hasAttribute('value') ? $node->getAttribute('value') : $defaultValue;
$option['disabled'] = ($node->hasAttribute('disabled') && $node->getAttribute('disabled') == 'disabled');

return $option;
}

/**
* Checks whether given vale is in the existing options
*
* @param string $optionValue
* @param array $options
*
* @return bool
*/
public function containsOption($optionValue, $options)
{
foreach ($options as $option) {
if ($option['value'] == $optionValue) {
return true;
}
}

return false;
}

/**
* Returns list of available field options
*
* @return array
*/
public function availableOptionValues()
{
$values = array();

foreach ($this->options as $option) {
$values[] = $option['value'];
}

return $values;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
use Symfony\Component\Config\Loader\LoaderInterface;
use Symfony\Component\Config\Loader\LoaderResolver;
use Symfony\Component\Config\Loader\LoaderResolverInterface;

/**
* AnnotationClassLoader loads routing information from a PHP class and its methods.
Expand Down Expand Up @@ -176,16 +176,16 @@ public function supports($resource, $type = null)
/**
* Sets the loader resolver.
*
* @param LoaderResolver $resolver A LoaderResolver instance
* @param LoaderResolverInterface $resolver A LoaderResolverInterface instance
*/
public function setResolver(LoaderResolver $resolver)
public function setResolver(LoaderResolverInterface $resolver)
{
}

/**
* Gets the loader resolver.
*
* @return LoaderResolver A LoaderResolver instance
* @return LoaderResolverInterface A LoaderResolverInterface instance
*/
public function getResolver()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ public function onKernelResponse(FilterResponseEvent $event)
if (HttpKernelInterface::MASTER_REQUEST !== $event->getRequestType()) {
return;
}

if (!$event->getRequest()->hasSession()) {
return;
}

if (null !== $this->logger) {
$this->logger->debug('Write SecurityContext in the session');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,20 @@ public function testRefreshUserGetsUserByPrimaryKey()

$this->assertSame($user1, $provider->refreshUser($user1));
}

public function testRefreshUserRequiresId()
{
$em = $this->createTestEntityManager();

$user1 = new CompositeIdentEntity(null, null, 'user1');
$provider = new EntityUserProvider($em, 'Symfony\Tests\Bridge\Doctrine\Fixtures\CompositeIdentEntity', 'name');

$this->setExpectedException(
'InvalidArgumentException',
'You cannot refresh a user from the EntityUserProvider that does not contain an identifier. The user object has to be serialized with its own identifier mapped by Doctrine'
);
$provider->refreshUser($user1);
}

private function createSchema($em)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,22 @@ public function testRadioButtons()
}
}

public function testRadioButtonIsDisabled()
{
$node = $this->createNode('input', '', array('type' => 'radio', 'name' => 'name', 'value' => 'foo', 'disabled' => 'disabled'));
$field = new ChoiceFormField($node);
$node = $this->createNode('input', '', array('type' => 'radio', 'name' => 'name', 'value' => 'bar'));
$field->addChoice($node);

$field->select('foo');
$this->assertEquals('foo', $field->getValue(), '->getValue() returns the value attribute of the selected radio button');
$this->assertTrue($field->isDisabled());

$field->select('bar');
$this->assertEquals('bar', $field->getValue(), '->getValue() returns the value attribute of the selected radio button');
$this->assertFalse($field->isDisabled());
}

public function testCheckboxes()
{
$node = $this->createNode('input', '', array('type' => 'checkbox', 'name' => 'name'));
Expand Down

2 comments on commit 3026287

@jmikola
Copy link
Contributor

@jmikola jmikola commented on 3026287 Dec 5, 2011

Choose a reason for hiding this comment

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

@fabpot: #2785 was made against 2.0, but it looks like you merged into master here.

@lsmith77
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm .. this is a BC break inside 2.0.x .. i personally don't mind since i can keep all mod my projects using the latest versions. but is this what we want to do?

Please sign in to comment.