Skip to content

Commit 9aee2ad

Browse files
webmozartwebmozart
committed
[PropertyAccess] Removed the argument $value from isWritable()
To keep isWritable() and setValue() consistent, the exception thrown when only an adder was present, but no remover (or vice versa), was removed.
1 parent 4262707 commit 9aee2ad

File tree

4 files changed

+143
-134
lines changed

4 files changed

+143
-134
lines changed

‎src/Symfony/Component/PropertyAccess/Exception/InvalidArgumentException.php‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <[email protected]>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\PropertyAccess\Exception;
13+
14+
/**
15+
* Base InvalidArgumentException for the PropertyAccess component.
16+
*
17+
* @author Bernhard Schussek <[email protected]>
18+
*/
19+
class InvalidArgumentException extends \InvalidArgumentException implements ExceptionInterface
20+
{
21+
}

‎src/Symfony/Component/PropertyAccess/PropertyAccessor.php‎

Lines changed: 110 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\PropertyAccess;
1313

14+
use Symfony\Component\PropertyAccess\Exception\InvalidArgumentException;
1415
use Symfony\Component\PropertyAccess\Exception\NoSuchPropertyException;
1516
use Symfony\Component\PropertyAccess\Exception\NoSuchIndexException;
1617
use Symfony\Component\PropertyAccess\Exception\UnexpectedTypeException;
@@ -53,7 +54,12 @@ public function getValue($objectOrArray, $propertyPath)
5354
if (is_string($propertyPath)) {
5455
$propertyPath = new PropertyPath($propertyPath);
5556
} elseif (!$propertyPath instanceof PropertyPathInterface) {
56-
throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\PropertyAccess\PropertyPathInterface');
57+
throw new InvalidArgumentException(sprintf(
58+
'The property path should be a string or an instance of '.
59+
'"Symfony\Component\PropertyAccess\PropertyPathInterface". '.
60+
'Got: "%s"',
61+
is_object($propertyPath) ? get_class($propertyPath) : gettype($propertyPath)
62+
));
5763
}
5864

5965
$propertyValues =& $this->readPropertiesUntil($objectOrArray, $propertyPath, $propertyPath->getLength(), $this->ignoreInvalidIndices);
@@ -69,7 +75,12 @@ public function setValue(&$objectOrArray, $propertyPath, $value)
6975
if (is_string($propertyPath)) {
7076
$propertyPath = new PropertyPath($propertyPath);
7177
} elseif (!$propertyPath instanceof PropertyPathInterface) {
72-
throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\PropertyAccess\PropertyPathInterface');
78+
throw new InvalidArgumentException(sprintf(
79+
'The property path should be a string or an instance of '.
80+
'"Symfony\Component\PropertyAccess\PropertyPathInterface". '.
81+
'Got: "%s"',
82+
is_object($propertyPath) ? get_class($propertyPath) : gettype($propertyPath)
83+
));
7384
}
7485

7586
$propertyValues =& $this->readPropertiesUntil($objectOrArray, $propertyPath, $propertyPath->getLength() - 1);
@@ -90,13 +101,11 @@ public function setValue(&$objectOrArray, $propertyPath, $value)
90101
}
91102

92103
$property = $propertyPath->getElement($i);
93-
//$singular = $propertyPath->singulars[$i];
94-
$singular = null;
95104

96105
if ($propertyPath->isIndex($i)) {
97106
$this->writeIndex($objectOrArray, $property, $value);
98107
} else {
99-
$this->writeProperty($objectOrArray, $property, $singular, $value);
108+
$this->writeProperty($objectOrArray, $property, $value);
100109
}
101110
}
102111

@@ -113,7 +122,12 @@ public function isReadable($objectOrArray, $propertyPath)
113122
if (is_string($propertyPath)) {
114123
$propertyPath = new PropertyPath($propertyPath);
115124
} elseif (!$propertyPath instanceof PropertyPathInterface) {
116-
throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\PropertyAccess\PropertyPathInterface');
125+
throw new InvalidArgumentException(sprintf(
126+
'The property path should be a string or an instance of '.
127+
'"Symfony\Component\PropertyAccess\PropertyPathInterface". '.
128+
'Got: "%s"',
129+
is_object($propertyPath) ? get_class($propertyPath) : gettype($propertyPath)
130+
));
117131
}
118132

119133
try {
@@ -132,12 +146,17 @@ public function isReadable($objectOrArray, $propertyPath)
132146
/**
133147
* {@inheritdoc}
134148
*/
135-
public function isWritable($objectOrArray, $propertyPath, $value)
149+
public function isWritable($objectOrArray, $propertyPath)
136150
{
137151
if (is_string($propertyPath)) {
138152
$propertyPath = new PropertyPath($propertyPath);
139153
} elseif (!$propertyPath instanceof PropertyPathInterface) {
140-
throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\PropertyAccess\PropertyPathInterface');
154+
throw new InvalidArgumentException(sprintf(
155+
'The property path should be a string or an instance of '.
156+
'"Symfony\Component\PropertyAccess\PropertyPathInterface". '.
157+
'Got: "%s"',
158+
is_object($propertyPath) ? get_class($propertyPath) : gettype($propertyPath)
159+
));
141160
}
142161

143162
try {
@@ -165,13 +184,12 @@ public function isWritable($objectOrArray, $propertyPath, $value)
165184
return false;
166185
}
167186
} else {
168-
if (!$this->isPropertyWritable($objectOrArray, $property, $value)) {
187+
if (!$this->isPropertyWritable($objectOrArray, $property)) {
169188
return false;
170189
}
171190
}
172191
}
173192

174-
$value = $objectOrArray;
175193
$overwrite = !$propertyValues[$i][self::IS_REF];
176194
}
177195

@@ -346,7 +364,7 @@ private function &readProperty(&$object, $property)
346364
}
347365

348366
/**
349-
* Sets the value of the property at the given index in the path
367+
* Sets the value of an index in a given array-accessible value.
350368
*
351369
* @param \ArrayAccess|array $array An array or \ArrayAccess object to write to
352370
* @param string|integer $index The index to write at
@@ -364,74 +382,33 @@ private function writeIndex(&$array, $index, $value)
364382
}
365383

366384
/**
367-
* Sets the value of the property at the given index in the path
385+
* Sets the value of a property in the given object
368386
*
369-
* @param object|array $object The object or array to write to
370-
* @param string $property The property to write
371-
* @param string|null $singular The singular form of the property name or null
372-
* @param mixed $value The value to write
387+
* @param object $object The object to write to
388+
* @param string $property The property to write
389+
* @param mixed $value The value to write
373390
*
374391
* @throws NoSuchPropertyException If the property does not exist or is not
375392
* public.
376393
*/
377-
private function writeProperty(&$object, $property, $singular, $value)
394+
private function writeProperty(&$object, $property, $value)
378395
{
379-
$guessedAdders = '';
380-
381396
if (!is_object($object)) {
382397
throw new NoSuchPropertyException(sprintf('Cannot write property "%s" to an array. Maybe you should write the property path as "[%s]" instead?', $property, $property));
383398
}
384399

385400
$reflClass = new \ReflectionClass($object);
386401
$plural = $this->camelize($property);
387-
388-
// Any of the two methods is required, but not yet known
389-
$singulars = null !== $singular ? array($singular) : (array) StringUtil::singularify($plural);
402+
$singulars = (array) StringUtil::singularify($plural);
390403

391404
if (is_array($value) || $value instanceof \Traversable) {
392405
$methods = $this->findAdderAndRemover($reflClass, $singulars);
393406

407+
// Use addXxx() and removeXxx() to write the collection
394408
if (null !== $methods) {
395-
// At this point the add and remove methods have been found
396-
// Use iterator_to_array() instead of clone in order to prevent side effects
397-
// see https://github.com/symfony/symfony/issues/4670
398-
$itemsToAdd = is_object($value) ? iterator_to_array($value) : $value;
399-
$itemToRemove = array();
400-
$propertyValue = $this->readProperty($object, $property);
401-
$previousValue = $propertyValue[self::VALUE];
402-
403-
if (is_array($previousValue) || $previousValue instanceof \Traversable) {
404-
foreach ($previousValue as $previousItem) {
405-
foreach ($value as $key => $item) {
406-
if ($item === $previousItem) {
407-
// Item found, don't add
408-
unset($itemsToAdd[$key]);
409-
410-
// Next $previousItem
411-
continue 2;
412-
}
413-
}
414-
415-
// Item not found, add to remove list
416-
$itemToRemove[] = $previousItem;
417-
}
418-
}
419-
420-
foreach ($itemToRemove as $item) {
421-
call_user_func(array($object, $methods[1]), $item);
422-
}
423-
424-
foreach ($itemsToAdd as $item) {
425-
call_user_func(array($object, $methods[0]), $item);
426-
}
409+
$this->writeCollection($object, $property, $value, $methods[0], $methods[1]);
427410

428411
return;
429-
} else {
430-
// It is sufficient to include only the adders in the error
431-
// message. If the user implements the adder but not the remover,
432-
// an exception will be thrown in findAdderAndRemover() that
433-
// the remover has to be implemented as well.
434-
$guessedAdders = '"add'.implode('()", "add', $singulars).'()", ';
435412
}
436413
}
437414

@@ -459,43 +436,98 @@ private function writeProperty(&$object, $property, $singular, $value)
459436
'Neither the property "%s" nor one of the methods %s"%s()", '.
460437
'"__set()" or "__call()" exist and have public access in class "%s".',
461438
$property,
462-
$guessedAdders,
439+
implode('', array_map(function ($singular) {
440+
return '"add'.$singular.'()"/"remove'.$singular.'()", ';
441+
}, $singulars)),
463442
$setter,
464443
$reflClass->name
465444
));
466445
}
467446
}
468447

469-
private function isPropertyWritable($object, $property, $value)
448+
/**
449+
* Adjusts a collection-valued property by calling add*() and remove*()
450+
* methods.
451+
*
452+
* @param object $object The object to write to
453+
* @param string $property The property to write
454+
* @param array|\Traversable $collection The collection to write
455+
* @param string $addMethod The add*() method
456+
* @param string $removeMethod The remove*() method
457+
*/
458+
private function writeCollection($object, $property, $collection, $addMethod, $removeMethod)
470459
{
471-
if (!is_object($object)) {
472-
throw new NoSuchPropertyException(sprintf('Cannot write property "%s" to an array. Maybe you should write the property path as "[%s]" instead?', $property, $property));
460+
// At this point the add and remove methods have been found
461+
// Use iterator_to_array() instead of clone in order to prevent side effects
462+
// see https://github.com/symfony/symfony/issues/4670
463+
$itemsToAdd = is_object($collection) ? iterator_to_array($collection) : $collection;
464+
$itemToRemove = array();
465+
$propertyValue = $this->readProperty($object, $property);
466+
$previousValue = $propertyValue[self::VALUE];
467+
468+
if (is_array($previousValue) || $previousValue instanceof \Traversable) {
469+
foreach ($previousValue as $previousItem) {
470+
foreach ($collection as $key => $item) {
471+
if ($item === $previousItem) {
472+
// Item found, don't add
473+
unset($itemsToAdd[$key]);
474+
475+
// Next $previousItem
476+
continue 2;
477+
}
478+
}
479+
480+
// Item not found, add to remove list
481+
$itemToRemove[] = $previousItem;
482+
}
473483
}
474484

475-
$reflClass = new \ReflectionClass($object);
476-
$plural = $this->camelize($property);
485+
foreach ($itemToRemove as $item) {
486+
call_user_func(array($object, $removeMethod), $item);
487+
}
477488

478-
// Any of the two methods is required, but not yet known
479-
$singulars = (array) StringUtil::singularify($plural);
489+
foreach ($itemsToAdd as $item) {
490+
call_user_func(array($object, $addMethod), $item);
491+
}
492+
}
480493

481-
if (is_array($value) || $value instanceof \Traversable) {
482-
try {
483-
if (null !== $this->findAdderAndRemover($reflClass, $singulars)) {
484-
return true;
485-
}
486-
} catch (NoSuchPropertyException $e) {
487-
return false;
488-
}
494+
/**
495+
* Returns whether a property is writable in the given object.
496+
*
497+
* @param object $object The object to write to
498+
* @param string $property The property to write
499+
*
500+
* @return Boolean Whether the property is writable
501+
*/
502+
private function isPropertyWritable($object, $property)
503+
{
504+
if (!is_object($object)) {
505+
return false;
489506
}
490507

508+
$reflClass = new \ReflectionClass($object);
509+
491510
$setter = 'set'.$this->camelize($property);
492511
$classHasProperty = $reflClass->hasProperty($property);
493512

494-
return $this->isMethodAccessible($reflClass, $setter, 1)
513+
if ($this->isMethodAccessible($reflClass, $setter, 1)
495514
|| $this->isMethodAccessible($reflClass, '__set', 2)
496515
|| ($classHasProperty && $reflClass->getProperty($property)->isPublic())
497516
|| (!$classHasProperty && property_exists($object, $property))
498-
|| ($this->magicCall && $this->isMethodAccessible($reflClass, '__call', 2));
517+
|| ($this->magicCall && $this->isMethodAccessible($reflClass, '__call', 2))) {
518+
return true;
519+
}
520+
521+
$plural = $this->camelize($property);
522+
523+
// Any of the two methods is required, but not yet known
524+
$singulars = (array) StringUtil::singularify($plural);
525+
526+
if (null !== $this->findAdderAndRemover($reflClass, $singulars)) {
527+
return true;
528+
}
529+
530+
return false;
499531
}
500532

501533
/**
@@ -517,8 +549,6 @@ private function camelize($string)
517549
* @param array $singulars The singular form of the property name or null
518550
*
519551
* @return array|null An array containing the adder and remover when found, null otherwise
520-
*
521-
* @throws NoSuchPropertyException If the property does not exist
522552
*/
523553
private function findAdderAndRemover(\ReflectionClass $reflClass, array $singulars)
524554
{
@@ -534,19 +564,6 @@ private function findAdderAndRemover(\ReflectionClass $reflClass, array $singula
534564
if ($addMethodFound && $removeMethodFound) {
535565
return array($addMethod, $removeMethod);
536566
}
537-
538-
if ($addMethodFound xor $removeMethodFound && null === $exception) {
539-
$exception = new NoSuchPropertyException(sprintf(
540-
'Found the public method "%s()", but did not find a public "%s()" on class %s',
541-
$addMethodFound ? $addMethod : $removeMethod,
542-
$addMethodFound ? $removeMethod : $addMethod,
543-
$reflClass->name
544-
));
545-
}
546-
}
547-
548-
if (null !== $exception) {
549-
throw $exception;
550567
}
551568

552569
return null;

0 commit comments

Comments
 (0)