Skip to content

[PurgatoryPropertyAccessor] fix usage of nullsafe operator with collection accessor #140

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.

Sign up for GitHub

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

Draft
Brajk19 wants to merge 1 commit into
base: 2.x
Choose a base branch
from
Draft

[PurgatoryPropertyAccessor] fix usage of nullsafe operator with collection accessor #140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 30 additions & 3 deletions src/RouteProvider/PropertyAccess/PurgatoryPropertyAccessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use Symfony\Component\PropertyAccess\Exception\AccessException;
use Symfony\Component\PropertyAccess\Exception\UnexpectedTypeException;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
use Symfony\Component\PropertyAccess\PropertyPath;
use Symfony\Component\PropertyAccess\PropertyPathInterface;

/**
Expand All @@ -34,10 +35,19 @@
/** @var array{0: string, 1: string} $propertyPathParts */
$propertyPathParts = explode(separator: self::DELIMITER, string: (string) $propertyPath, limit: 2);

$collection = $this->propertyAccessor->getValue($objectOrArray, $propertyPathParts[0]);
$basePropertyPath = new PropertyPath($propertyPathParts[0]);
$collection = $this->propertyAccessor->getValue($objectOrArray, $basePropertyPath);

if (!is_iterable($collection)) {
throw new ValueNotIterableException($collection, $propertyPathParts[0]);
// Honor the null-safe operator: when the collection segment resolves to null because of
// a null-safe short-circuit (e.g. "property?.collection[*].id" with a null "property"),
// yield no values instead of throwing. A null collection not guarded by "?."
// (e.g. "property.collection[*].id" where "collection" itself is null) is still an error.
if (null === $collection && $this->isNullSafeCollection($objectOrArray, $basePropertyPath)) {
return null;
}

throw new ValueNotIterableException($collection, (string) $basePropertyPath);
}

$values = [];
Expand Down Expand Up @@ -87,8 +97,25 @@
$this->getValue($objectOrArray, $propertyPath);

return true;
} catch (AccessException|UnexpectedTypeException) {
} catch (AccessException|UnexpectedTypeException|ValueNotIterableException) {

Check failure on line 100 in src/RouteProvider/PropertyAccess/PurgatoryPropertyAccessor.php

View workflow job for this annotation

GitHub Actions / Static Analysis 

Dead catch - Sofascore\PurgatoryBundle\Exception\ValueNotIterableException is never thrown in the try block.
return false;
}
}

/**
* Determines whether a null collection segment is the legitimate result of a null-safe ("?.")
* short-circuit, rather than the collection itself resolving to null.
*
* @param object|array<array-key, mixed> $objectOrArray
*/
private function isNullSafeCollection(object|array $objectOrArray, PropertyPath $path): bool
{
if ($path->isNullSafe($path->getLength() - 1)) {
return true;
}

$parent = $path->getParent();

return null !== $parent && null === $this->propertyAccessor->getValue($objectOrArray, $parent);
}
}
2 changes: 2 additions & 0 deletions tests/RouteProvider/PropertyAccess/Fixtures/Foo.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ class Foo
public function __construct(
public readonly int $id,
public readonly Collection $children,
public readonly ?self $linked = null,
public readonly ?Collection $nullableChildren = null,
) {
}

Expand Down
116 changes: 110 additions & 6 deletions tests/RouteProvider/PropertyAccess/PurgatoryPropertyAccessorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,17 +130,121 @@ public static function traversableProvider(): iterable
];
}

public function testNotTraversable(): void
#[DataProvider('nullSafeProvider')]
public function testNullSafeCollectionPath(object $object, string $propertyPath, mixed $expectedResult): void
{
self::assertTrue($this->purgatoryPropertyAccessor->isReadable($object, $propertyPath));
self::assertSame(
expected: $expectedResult,
actual: $this->purgatoryPropertyAccessor->getValue($object, $propertyPath),
);
}

public static function nullSafeProvider(): iterable
{
yield 'null-safe parent short-circuits to null' => [
'object' => new Foo(
id: 100,
children: new ArrayCollection([]),
linked: null,
),
'propertyPath' => 'linked?.children[*].id',
'expectedResult' => null,
];

yield 'null-safe parent present with populated collection yields values' => [
'object' => new Foo(
id: 100,
children: new ArrayCollection([]),
linked: new Foo(
id: 1,
children: new ArrayCollection([
new Foo(id: 10, children: new ArrayCollection([])),
new Foo(id: 11, children: new ArrayCollection([])),
]),
),
),
'propertyPath' => 'linked?.children[*].id',
'expectedResult' => [10, 11],
];

yield 'null-safe collection element resolving to null short-circuits to null' => [
'object' => new Foo(
id: 100,
children: new ArrayCollection([]),
nullableChildren: null,
),
'propertyPath' => 'nullableChildren?[*].id',
'expectedResult' => null,
];

yield 'nested null-safe short-circuit contributes a null entry, consistent with a scalar leaf' => [
'object' => new Foo(
id: 0,
children: new ArrayCollection([
new Foo(id: 1, children: new ArrayCollection([]), linked: null),
new Foo(
id: 2,
children: new ArrayCollection([]),
linked: new Foo(
id: 20,
children: new ArrayCollection([
new Foo(id: 100, children: new ArrayCollection([])),
new Foo(id: 101, children: new ArrayCollection([])),
]),
),
),
]),
),
'propertyPath' => 'children[*].linked?.children[*].id',
'expectedResult' => [null, 100, 101],
];
}

#[DataProvider('notTraversableProvider')]
public function testNotTraversableThrows(object $object, string $propertyPath, string $expectedMessage): void
{
self::assertFalse($this->purgatoryPropertyAccessor->isReadable($object, $propertyPath));

$this->expectException(ValueNotIterableException::class);
$this->expectExceptionMessage('Expected an iterable, "int" given at property path "id[*]".');
$this->expectExceptionMessage($expectedMessage);

$this->purgatoryPropertyAccessor->getValue(
objectOrArray: new Foo(
$this->purgatoryPropertyAccessor->getValue($object, $propertyPath);
}

public static function notTraversableProvider(): iterable
{
yield 'scalar value is not iterable' => [
'object' => new Foo(
id: 1,
children: new ArrayCollection([]),
),
propertyPath: 'id[*].id',
);
'propertyPath' => 'id[*].id',
'expectedMessage' => 'Expected an iterable, "int" given at property path "id[*]".',
];

yield 'non-null-safe collection resolving to null' => [
'object' => new Foo(
id: 100,
children: new ArrayCollection([]),
nullableChildren: null,
),
'propertyPath' => 'nullableChildren[*].id',
'expectedMessage' => 'Expected an iterable, "null" given at property path "nullableChildren[*]".',
];

yield 'null-safe parent present but non-null-safe collection resolving to null' => [
'object' => new Foo(
id: 100,
children: new ArrayCollection([]),
linked: new Foo(
id: 1,
children: new ArrayCollection([]),
nullableChildren: null,
),
),
'propertyPath' => 'linked?.nullableChildren[*].id',
'expectedMessage' => 'Expected an iterable, "null" given at property path "linked?.nullableChildren[*]".',
];
}
}
Loading