-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[2.4][PropertyAccess] Fixed getValue() when accessing non-existing indices of ArrayAccess implementations #10947
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,7 +33,7 @@ class PropertyAccessor implements PropertyAccessorInterface | |
/** | ||
* @var bool | ||
*/ | ||
private $throwExceptionOnInvalidIndex; | ||
private $ignoreInvalidIndices; | ||
|
||
/** | ||
* Should not be used by application code. Use | ||
|
@@ -42,7 +42,7 @@ class PropertyAccessor implements PropertyAccessorInterface | |
public function __construct($magicCall = false, $throwExceptionOnInvalidIndex = false) | ||
{ | ||
$this->magicCall = $magicCall; | ||
$this->throwExceptionOnInvalidIndex = $throwExceptionOnInvalidIndex; | ||
$this->ignoreInvalidIndices = $throwExceptionOnInvalidIndex; | ||
} | ||
|
||
/** | ||
|
@@ -56,7 +56,7 @@ public function getValue($objectOrArray, $propertyPath) | |
throw new UnexpectedTypeException($propertyPath, 'string or Symfony\Component\PropertyAccess\PropertyPathInterface'); | ||
} | ||
|
||
$propertyValues =& $this->readPropertiesUntil($objectOrArray, $propertyPath, $propertyPath->getLength(), $this->throwExceptionOnInvalidIndex); | ||
$propertyValues =& $this->readPropertiesUntil($objectOrArray, $propertyPath, $propertyPath->getLength(), $this->ignoreInvalidIndices); | ||
|
||
return $propertyValues[count($propertyValues) - 1][self::VALUE]; | ||
} | ||
|
@@ -116,7 +116,7 @@ public function setValue(&$objectOrArray, $propertyPath, $value) | |
* | ||
* @throws UnexpectedTypeException If a value within the path is neither object nor array. | ||
*/ | ||
private function &readPropertiesUntil(&$objectOrArray, PropertyPathInterface $propertyPath, $lastIndex, $throwExceptionOnNonexistantIndex = false) | ||
private function &readPropertiesUntil(&$objectOrArray, PropertyPathInterface $propertyPath, $lastIndex, $ignoreInvalidIndices = false) | ||
{ | ||
$propertyValues = array(); | ||
|
||
|
@@ -131,9 +131,25 @@ private function &readPropertiesUntil(&$objectOrArray, PropertyPathInterface $pr | |
|
||
// Create missing nested arrays on demand | ||
if ($isIndex && $isArrayAccess && !isset($objectOrArray[$property])) { | ||
if ($throwExceptionOnNonexistantIndex) { | ||
throw new NoSuchIndexException(sprintf('Cannot read property "%s". Available properties are "%s"', $property, print_r(array_keys($objectOrArray), true))); | ||
if ($ignoreInvalidIndices) { | ||
if (!is_array($objectOrArray)) { | ||
if (!$objectOrArray instanceof \Traversable) { | ||
throw new NoSuchIndexException(sprintf( | ||
'Cannot read property "%s".', | ||
$property | ||
)); | ||
} | ||
|
||
$objectOrArray = iterator_to_array($objectOrArray); | ||
} | ||
|
||
throw new NoSuchIndexException(sprintf( | ||
'Cannot read property "%s". Available properties are "%s"', | ||
$property, | ||
print_r(array_keys($objectOrArray), true) | ||
)); | ||
} | ||
|
||
$objectOrArray[$property] = $i + 1 < $propertyPath->getLength() ? array() : null; | ||
} | ||
|
||
|
@@ -412,8 +428,8 @@ private function findAdderAndRemover(\ReflectionClass $reflClass, array $singula | |
$addMethod = 'add'.$singular; | ||
$removeMethod = 'remove'.$singular; | ||
|
||
$addMethodFound = $this->isAccessible($reflClass, $addMethod, 1); | ||
$removeMethodFound = $this->isAccessible($reflClass, $removeMethod, 1); | ||
$addMethodFound = $this->isMethodAccessible($reflClass, $addMethod, 1); | ||
$removeMethodFound = $this->isMethodAccessible($reflClass, $removeMethod, 1); | ||
|
||
if ($addMethodFound && $removeMethodFound) { | ||
return array($addMethod, $removeMethod); | ||
|
@@ -428,6 +444,8 @@ private function findAdderAndRemover(\ReflectionClass $reflClass, array $singula | |
)); | ||
} | ||
} | ||
|
||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be reverted There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree. Methods with a return value should always return a value, even if it is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @webmozart I see you missed one of 'strangest' PRs in Symfony =) #10717. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for pointing that out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you revert this change please. This changes has been done everywhere now in all versions of Symfony, and we are not going to revert this. Thanks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @webmozart To be honest, I also prefer the explicit return, and I keep using it in my own projects. However, the code of Symfony should follow the rules stated for Symfony. |
||
} | ||
|
||
/** | ||
|
@@ -440,7 +458,7 @@ private function findAdderAndRemover(\ReflectionClass $reflClass, array $singula | |
* @return bool Whether the method is public and has $parameters | ||
* required parameters | ||
*/ | ||
private function isAccessible(\ReflectionClass $class, $methodName, $parameters) | ||
private function isMethodAccessible(\ReflectionClass $class, $methodName, $parameters) | ||
{ | ||
if ($class->hasMethod($methodName)) { | ||
$method = $class->getMethod($methodName); | ||
|
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
<?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\Component\PropertyAccess\Tests\Fixtures; | ||
|
||
/** | ||
* This class is a hand written simplified version of PHP native `ArrayObject` | ||
* class, to show that it behaves differently than the PHP native implementation. | ||
*/ | ||
class NonTraversableArrayObject implements \ArrayAccess, \Countable, \Serializable | ||
{ | ||
private $array; | ||
|
||
public function __construct(array $array = null) | ||
{ | ||
$this->array = $array ?: array(); | ||
} | ||
|
||
public function offsetExists($offset) | ||
{ | ||
return array_key_exists($offset, $this->array); | ||
} | ||
|
||
public function offsetGet($offset) | ||
{ | ||
return $this->array[$offset]; | ||
} | ||
|
||
public function offsetSet($offset, $value) | ||
{ | ||
if (null === $offset) { | ||
$this->array[] = $value; | ||
} else { | ||
$this->array[$offset] = $value; | ||
} | ||
} | ||
|
||
public function offsetUnset($offset) | ||
{ | ||
unset($this->array[$offset]); | ||
} | ||
|
||
public function count() | ||
{ | ||
return count($this->array); | ||
} | ||
|
||
public function serialize() | ||
{ | ||
return serialize($this->array); | ||
} | ||
|
||
public function unserialize($serialized) | ||
{ | ||
$this->array = (array) unserialize((string) $serialized); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be
$this->ignoreInvalidIndices = !$throwExceptionOnInvalidIndex;
, no?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah of course, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#10957