8000 [2.4][PropertyAccess] Fixed getValue() when accessing non-existing indices of ArrayAccess implementations by webmozart · Pull Request #10947 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Merged
merged 3 commits into from
May 21, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions src/Symfony/Component/PropertyAccess/PropertyAccessor.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class PropertyAccessor implements PropertyAccessorInterface
/**
* @var bool
*/
private $throwExceptionOnInvalidIndex;
private $ignoreInvalidIndices;

/**
* Should not be used by application code. Use
Expand All @@ -42,7 +42,7 @@ class PropertyAccessor implements PropertyAccessorInterface
public function __construct($magicCall = false, $throwExceptionOnInvalidIndex = false)
{
$this->magicCall = $magicCall;
$this->throwExceptionOnInvalidIndex = $throwExceptionOnInvalidIndex;
$this->ignoreInvalidIndices = $throwExceptionOnInvalidIndex;
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah of course, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

/**
Expand All @@ -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];
}
Expand Down Expand Up @@ -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();

Expand All @@ -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;
}

Expand Down Expand Up @@ -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);
Expand All @@ -428,6 +444,8 @@ private function findAdderAndRemover(\ReflectionClass $reflClass, array $singula
));
}
}

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.

should be reverted

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 null. Relying on the implicit return value is confusing when reading the code (was the return statement forgotten?) - which was exactly what happened to me now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@webmozart I see you missed one of 'strangest' PRs in Symfony =) #10717.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

}

/**
Expand All @@ -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);
Expand Down
71 changes: 0 additions & 71 deletions src/Symfony/Component/PropertyAccess/Tests/Fixtures/Author.php

This file was deleted.

27 changes: 0 additions & 27 deletions src/Symfony/Component/PropertyAccess/Tests/Fixtures/Magician.php

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);
}
}
Loading
0