FEATURE: Add PropertyValueCriteriaMatcher

[mficzel]

The property value criteria matcher accepts a PropertyValueCriteriaInterface and provides static methods to check wether a Node or a PropertyCollection matches those criteria. This allows to use PHP for filtering nodes that were already fetched using the cr-property criteria.

Notes:

• The PropertyValueCriteriaMatcher operates on the serialized values to be as close to the database operations as possible.
• The PropertyValueCriteriaMatcher is merked @internal for now as is the the NodeType/ConstraintCheck that does very similar things.

Upgrade instructions

Review instructions

The review should check carefully that the implementation is in line with the checks in the db-drivers.

Also please note that i tried to write this as match statement but die not manage to get readable results for values that have to be read to a variable before checking.

Checklist

• Code follows the PSR-2 coding style
• Tests have been created, run and adjusted as needed
• The PR is created against the lowest maintained branch
• Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
• Reviewer - The first section explains the change briefly for change-logs
• Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

[mhsdesign]

I really like the idea but im not sure how we could check if its working as intended ... wait yes i do!!!

We could adjust the subgraph implementation of a few methods to not the db filtering but filter the nodes via php and see if the e2e test like it ^^

[mficzel]

Maybe the proposed inMemoryGraph has a use for this aswell :-)

[mhsdesign]

So i did it exactly as described above in #4710

and i noticed two kind of failing tests:

Not using strict === comparison in PropertyValueEquals

neos-development-collection/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeTraversal/DescendantNodes.feature

Line 112 in 5917d8b

When I execute the findDescendantNodes query for entry node aggregate id "home" and filter '{"propertyValue": "stringProperty = true"}' I expect no nodes to be returned

descendantNodes.feature
      When I execute the findDescendantNodes query for entry node aggregate id "home" and filter '{"propertyValue": "stringProperty = true"}' I expect no nodes to be returned # Features/NodeTraversal/DescendantNodes.feature:112
        findDescendantNodes returned an unexpected result
        Failed asserting that two arrays are identical.
        --- Expected
        +++ Actual
        @@ @@
        -Array &0 ()
        +Array &0 (
        +    0 => 'b'
        +    1 => 'b1'
        +)

String comparison is lowercase in mariadb in PropertyValueContains

https://github.com/neos/neos-development-collection/blob/5917d8b357b1c867c66ac949f72a34266df692d6/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/NodeTraversal/DescendantNodes.feature#L125C1-L126C1

001 Scenario:                                                                                                                                                                                                       # Features/NodeTraversal/DescendantNodes.feature:100
      When I execute the findDescendantNodes query for entry node aggregate id "home" and filter '{"propertyValue": "stringProperty *= \"späci\" OR text $= \"a1\""}' I expect the nodes "b,a1,a2a1" to be returned # Features/NodeTraversal/DescendantNodes.feature:126
        findDescendantNodes returned an unexpected result
        Failed asserting that two arrays are identical.
        --- Expected
        +++ Actual
        @@ @@
         Array &0 (
        -    0 => 'b'
        -    1 => 'a1'
        -    2 => 'a2a1'
        +    0 => 'a1'
        +    1 => 'a2a1'
         )

---

you can find my adjustments here
63a1e0a - if the tests also pass in ci, ill add them to your pr

[mficzel]

I am not sure we would consider the MariaDB behavior correct by default. Imho we should discuss what we consider correct here.

[mhsdesign]

Additionally i like to discuss the location of this matcher. With ExpandedNodeTypeCriteria we were also unsure where to place it but eventually considered it okay under Filters as its only supposed to be used by the subgraph impl.

But now it seems like we want to use it at other places as well - similar to PropertyValueCriteriaMatcher ^^

What do you say @bwaidelich?

[mhsdesign]

And Postgres was exactly the opposite where its always case sensitive? isnt it?

[mficzel]

@nezaniel can you help us to determine what the best default should be. Personally think the php implementation should be the best match and db-drivers should try to be as compatible as conveniently possible. Imho it is fine that using a specific database can have pros and cons.

[mficzel]

Converted to draft as the case-sensitivity of the property value operations still has to be defined. Until that is done this is on hold.

[bwaidelich]

Converted to draft as the case-sensitivity of the property value operations still has to be defined. Until that is done this is on hold.

See #4721

[mhsdesign]

With the case insensitive matches merged: #4725 this is now unblocked.

[mficzel]

With #4721 solved this is updated and now handles case sensitivity.

[bwaidelich]

Great one, thanks!
Just a comment re switch vs match

[mhsdesign]

As last time i again run the PropertyValueCriteriaMatcher against the behavioural tests by using it for filtering instead the db in the ContentSubgraphWithRuntimeCaches (which would be then rather a SlowContentSubgraph implementation)

The previous matter of case sensitivity was resolved, but the also mentioned issue Not using strict === comparison in PropertyValueEquals in propertyValueEquals still persists.

001 Scenario:                                                                                                                                                                  # Features/NodeTraversal/DescendantNodes.feature:100
      When I execute the findDescendantNodes query for entry node aggregate id "home" and filter '{"propertyValue": "stringProperty = true"}' I expect no nodes to be returned # Features/NodeTraversal/DescendantNodes.feature:112
        findDescendantNodes returned an unexpected result
        Failed asserting that two arrays are equal.
        --- Expected
        +++ Actual
        @@ @@
         Array (
        +    0 => 'b'
        +    1 => 'b1'
         )

The inline doc also specifically denotes that strict comparison should be used

The following should not return node "b1" because boolean true !== "true"

Testrunner: #4898

[mhsdesign]

i like to propose to use here as first parameter the serialised node properties directly. Using the higher level object is not necessary and might even introduce bugs as the matching should only happen on the serialised properties. Also it will be easier to test.

A last testrun is now triggered at #4898 but i assume everything will be green and youll get my approval :D

[mficzel]

Not sure this is an actual benefit. In which usecase would one have serialized properties but not the propertyCollection.

[mficzel]

Introduded matchesSerializedPropertyValues as separate method that is used by matchNode and matchPropertyCollection

[mhsdesign]

Thänks. My reasoning here was just simply that when developing i came mostly across the SerializedPropertyValues and not the abstraction of the PropertyCollection which is not always available and harder to construct as it needs the serialiser. Also by passing in the serialised properties it becomes more obvious that the filtering happens on that level..

[mhsdesign]

Tests at #4898 pass :D So its now basically a 1 to 1 implementation as far as the e2e testsuite says :)

[mhsdesign]

Thank you for the adjustments and fixing everything.