-
-
Notifications
You must be signed in to change notification settings - Fork 224
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
!!! BUGFIX: Consider entry node when filtering in reference editor #5195
Changes from 6 commits
9084d69
989e7c5
ef93f94
4715f33
3bbabb3
5a8ab49
5bce349
0190e16
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 |
---|---|---|
|
@@ -38,8 +38,6 @@ private function __construct( | |
} | ||
|
||
/** | ||
* If the value is NULL an unset-property instruction will be returned instead. | ||
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. unrelated and otherwise not changed file 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 know yeah that the comment is false was my fault and i fixed it ^^ |
||
* | ||
* @param Value $value | ||
*/ | ||
public static function create( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,7 @@ | |
|
||
declare(strict_types=1); | ||
|
||
namespace Neos\ContentRepository\Core\Projection\ContentGraph; | ||
namespace Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm; | ||
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 think, we had it in the main ContentGraph folder by intention because it is likely to be reused for other parts. 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. Yep, I would also let it stay where it was. 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. With the rest of the changes I came to understand why you moved it 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 would almost argue the SearchTermMatcher doesn't belong in Projection/ContentGraph/Filters and that strengthens the argument the SearchTerm should be generic as well. Yes the SearchTermMatcher applies to contentgraph projection nodes, BUT I tihnk it's clear that everything else in filters is used in conjunction with a ContentGraph or Subgraph query (query in the loosest term not necessarily DB query), while the SearchTermMatcher applies to a set of nodes that have already been assembled. I think that is its own kind of operation. 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 moved it to align it to the |
||
|
||
/** | ||
* A search term for use in Filters for the {@see ContentSubgraphInterface} API. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm; | ||
|
||
use Neos\ContentRepository\Core\Feature\NodeModification\Dto\SerializedPropertyValue; | ||
use Neos\ContentRepository\Core\Feature\NodeModification\Dto\SerializedPropertyValues; | ||
use Neos\ContentRepository\Core\Projection\ContentGraph\Node; | ||
|
||
/** | ||
* Performs search term check against the nodes properties | ||
* | ||
* @internal | ||
*/ | ||
class SearchTermMatcher | ||
mhsdesign marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
public static function matchesNode(Node $node, SearchTerm $searchTerm): bool | ||
{ | ||
return static::matchesSerializedPropertyValues($node->properties->serialized(), $searchTerm); | ||
} | ||
|
||
public static function matchesSerializedPropertyValues(SerializedPropertyValues $serializedPropertyValues, SearchTerm $searchTerm): bool | ||
{ | ||
foreach ($serializedPropertyValues as $serializedPropertyValue) { | ||
if (self::matchesValue($serializedPropertyValue->value, $searchTerm)) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
private static function matchesValue(mixed $value, SearchTerm $searchTerm): bool | ||
{ | ||
if (is_array($value) || $value instanceof \ArrayObject) { | ||
foreach ($value as $subValue) { | ||
if (self::matchesValue($subValue, $searchTerm)) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
return match (true) { | ||
is_string($value) => mb_stripos($value, $searchTerm->term) !== false, | ||
// the following behaviour might seem odd, but is implemented after how the database filtering should behave | ||
is_int($value), | ||
is_float($value) => str_contains((string)$value, $searchTerm->term), | ||
$value === true => $searchTerm->term === 'true', | ||
$value === false => $searchTerm->term === 'false', | ||
default => throw new \InvalidArgumentException(sprintf('Handling for type %s is not implemented.', get_debug_type($value))), | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,6 +124,16 @@ public function merge(self $other): self | |
return self::fromArray($nodes); | ||
} | ||
|
||
public function prepend(Node $node): self | ||
{ | ||
return new self([$node, ...$this->nodes]); | ||
} | ||
|
||
public function append(Node $node): self | ||
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. Why not use merge? 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. With merge I would expect an instance of 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. True, but it wouldn't be the first place we use a 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. In other places we named it more explicitly, like NodeTypeNames::withAdditionalNodeTypeName().
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. btw: I think 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. Yep, was mostly arguing for remembering to do it everywhere like this :) I am fine with having it, and yeah in htis case 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. Adjusted as requested :) 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. Sorry for re-opening, but when I suggested "Prepend" what I actually meant was 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. ahh i see for collections the pattern as far as i have seen is to have |
||
{ | ||
return new self([...$this->nodes, $node]); | ||
} | ||
|
||
public function reverse(): self | ||
{ | ||
return new self(array_reverse($this->nodes)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,161 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Neos\ContentRepository\Core\Tests\Unit\Projection\ContentGraph\Filter\SearchTerm; | ||
|
||
use Neos\ContentRepository\Core\Feature\NodeModification\Dto\SerializedPropertyValue; | ||
use Neos\ContentRepository\Core\Feature\NodeModification\Dto\SerializedPropertyValues; | ||
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTerm; | ||
use Neos\ContentRepository\Core\Projection\ContentGraph\Filter\SearchTerm\SearchTermMatcher; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
class SearchTermMatcherTest extends TestCase | ||
{ | ||
public static function matchingStringComparisonExamples(): iterable | ||
{ | ||
yield 'string found inside string' => ['brown', self::value('the brown fox')]; | ||
yield 'string found inside string (ci)' => ['BrOWn', self::value('the brown fox')]; | ||
|
||
yield 'string found inside multibyte string (ci)' => ['Äpfel', self::value('Auer schreit der bauer die äpfel sind zu sauer.')]; | ||
|
||
yield 'string matches full string' => ['Sheep', self::value('Sheep')]; | ||
yield 'string matches full string (ci)' => ['sheep', self::value('Sheep')]; | ||
|
||
yield 'string found inside string with special chars' => ['ä b-c+#@', self::value('the example: "ä b-c+#@"')]; | ||
} | ||
|
||
public static function matchingNumberLikeComparisonExamples(): iterable | ||
{ | ||
yield 'string-number found inside string' => [ | ||
'22', | ||
self::value('feeling like 22 ;)'), | ||
]; | ||
|
||
yield 'string-number found inside string-number' => [ | ||
'00', | ||
self::value('007'), | ||
]; | ||
|
||
yield 'string-number found inside int' => [ | ||
'23', | ||
self::value(1234), | ||
]; | ||
|
||
yield 'string-number found inside float' => [ | ||
'23', | ||
self::value(1234.56), | ||
]; | ||
|
||
yield 'string-float matches float' => [ | ||
'1234.56', | ||
self::value(1234.56), | ||
]; | ||
|
||
yield 'string-int matches int' => [ | ||
'0', | ||
self::value(0), | ||
]; | ||
} | ||
|
||
public static function matchingBooleanLikeComparisonExamples(): iterable | ||
{ | ||
yield 'string-boolean inside string' => [ | ||
'true', | ||
self::value('this is true'), | ||
]; | ||
|
||
yield 'string-true matches boolean' => [ | ||
'true', | ||
self::value(true), | ||
]; | ||
|
||
yield 'string-false matches boolean' => [ | ||
'false', | ||
self::value(false), | ||
]; | ||
} | ||
|
||
public static function matchingArrayComparisonExamples(): iterable | ||
{ | ||
// automates the following: | ||
yield 'inside array: string found inside string' => ['foo', self::value(['foo'])]; | ||
yield 'inside array-object: string found inside string' => ['foo', self::value(new \ArrayObject(['foo']))]; | ||
|
||
foreach([ | ||
...iterator_to_array(self::matchingStringComparisonExamples()), | ||
...iterator_to_array(self::matchingNumberLikeComparisonExamples()), | ||
...iterator_to_array(self::matchingBooleanLikeComparisonExamples()), | ||
] as $name => [$searchTerm, $properties]) { | ||
/** @var SerializedPropertyValues $properties */ | ||
yield 'inside nested array: ' . $name => [$searchTerm, SerializedPropertyValues::fromArray( | ||
array_map( | ||
fn (SerializedPropertyValue $value) => SerializedPropertyValue::create( | ||
// arbitrary deep nested | ||
[[$value->value]], | ||
'array' | ||
), | ||
iterator_to_array($properties) | ||
) | ||
)]; | ||
} | ||
} | ||
|
||
|
||
public function notMatchingExamples(): iterable | ||
{ | ||
yield 'different chars' => ['aepfel', self::value('äpfel')]; | ||
yield 'upper boolean string representation' => ['TRUE', self::value(true)]; | ||
yield 'string not found inside string' => ['reptv', self::value('eras tour')]; | ||
yield 'integer' => ['0999', self::value(999)]; | ||
yield 'float with comma' => ['12,45', self::value(12.34)]; | ||
yield 'array with unmatched string' => ['hello', self::value(['hi'])]; | ||
yield 'array key is not considered matching' => ['key', self::value(['key' => 'foo'])]; | ||
yield 'nested array key is not considered matching' => ['key', self::value([['key' => 'foo']])]; | ||
} | ||
|
||
/** | ||
* @test | ||
* @dataProvider matchingStringComparisonExamples | ||
* @dataProvider matchingNumberLikeComparisonExamples | ||
* @dataProvider matchingBooleanLikeComparisonExamples | ||
* @dataProvider matchingArrayComparisonExamples | ||
*/ | ||
public function searchTermMatchesProperties( | ||
string $searchTerm, | ||
SerializedPropertyValues $properties, | ||
) { | ||
self::assertTrue( | ||
SearchTermMatcher::matchesSerializedPropertyValues( | ||
$properties, | ||
SearchTerm::fulltext($searchTerm) | ||
) | ||
); | ||
} | ||
|
||
/** | ||
* @test | ||
* @dataProvider notMatchingExamples | ||
*/ | ||
public function searchTermDoesntMatchesProperties( | ||
string $searchTerm, | ||
SerializedPropertyValues $properties, | ||
) { | ||
self::assertFalse( | ||
SearchTermMatcher::matchesSerializedPropertyValues( | ||
$properties, | ||
SearchTerm::fulltext($searchTerm) | ||
) | ||
); | ||
} | ||
|
||
private static function value(string|bool|float|int|array|\ArrayObject $value): SerializedPropertyValues | ||
{ | ||
return SerializedPropertyValues::fromArray([ | ||
'test-property' => SerializedPropertyValue::create( | ||
$value, | ||
'' | ||
), | ||
]); | ||
} | ||
} |
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.
😆