9.0 TASK: remove string union types in filters, to increase type safety

[skurfuerst]

No description provided.

[skurfuerst]

From @mficzel :

I might get the context wrong but afaik “string” does not allow stringable objects if declare strict types is set. That would only be the case if the type was declared as “Class|stringable|string” … (edited)

[mhsdesign]

Lets build a property mapper for this job ;) :D

[bwaidelich]

Sweet! Did you check if existing code still uses the string versions?

[bwaidelich]

I had enabled strict mode for all filters and that indeed doesn't allow stringable objects.
But I'm still in favor of the strictly typed arguments. Especially to prevent issues with dynamically applied params

[mficzel]

Not against strictness. Just wanted to ensure we do not decide with a not fully correct assumption.

I would suggest to use the principle of beeing strict inside the CR but be a little more tolerant on the border to integration country (Eel, flowQuery). Offcourse only where there is no doubt which id is meant.

[bwaidelich]

I had enabled strict mode for all filters and that indeed doesn't allow stringable objects.

I have to correct myself.
The strict type is indeed relevant for the calling code only, see:
https://phpsandbox.io/n/string-casting-cpkuh?files=%2Funstrict.php

So

$filter->withNodeAggregateId(ContentStreamId::create());

Will indeed not fail if:

• The code is called in a file without declare(strict_types=1)
• ContentStreamId implements __toString() (which I would suggest to remove for this reason)

The tests currently fail because the old version was still used.
Especially in the Behat tests it was really easy to specify arbitrary filters and map them to the DTOs by spreading them like SomeFilter::create()->with(...$filterValues).
And that feature is actually extremely helpful when using the filters in a dynamic fashion (i.e. mapping query arguments to corresponding filters).
For this reason – and after thinking about it a little more – I suggest to take another route:

1. Revert this change and allow simple types in the filter constructors again (but always convert them immediately as before)
2. Make the VOs stricter where it makes sense (e.g. NodeTypeConstraints have to follow a certain pattern)

Strict VOs are anyways a huge help to prevent nasty bugs and security issues down the road. We can always relax the rules, but making them more strict won't be easily possible after a release.

BTW: I totally agree to @mficzel's comment re

I would suggest to use the principle of beeing strict inside the CR but be a little more tolerant on the border

And I would consider a Filter DTO to be on the border

[bwaidelich]

I discussed this again with @skurfuerst and we agreed to keep support for the literal types in the with method (instead of adding yet another builder or factory)

BTW: With #4133 we plan to remove __toString()-methods from VOs so that the accidental type casting is prevented