Report an error for QueryBuilder::setParameter without specifying the Type.

[VincentLanglet]

The idea would be to introduce a rule similar to psalm/psalm-plugin-symfony#158 in psalm.

According to the psalm issue and the doctrine related issue doctrine/orm#8113 there is a performance issue when passing an object as parameter without a Type, and it seems there is always some extra-computation when the type is not directly specificied
https://github.com/doctrine/orm/blob/8c259ea5cb632dbb57001b2262048ae7fa52b102/lib/Doctrine/ORM/Query.php#L409-L438

There is also an issue opened to deprecate the TypeInference in doctrine/orm and ask it to be explicit doctrine/orm#8379

Phpstan could ask to change something like

$qb->setParameter('foo', $foo)

to

$qb->setParameter('foo', $foo, $type)

Same could be done for new Parameter()

Since this might be opinionated, this could be an optional rule with a config to enable to this plugin.
WDYT @ondrejmirtes does such option would be accepted or should I implement this rule on my personal project ?

[ondrejmirtes]

Do you think this is a good idea @derrabus?

I also need to hear from you how you'd approach this and if it's even possible to detect the right type to use. From your message it's not obvious if you just always want to make the 3rd parameter required, or if you want to detect when the type MUST be used and WHAT type should be used. It'd probably prevent more bugs (but might be hard to do).

[janedbal]

Just FYI, we have similar rule in our codebase. We have a whitelist of safe objects that can be used without type specified (its __toString behaves as we want; this is the default Doctrine's behaviour). Everything else (objects only) gets reported if you dont specify 3rd argument.

[janedbal]

Also, there is a risk of mismatch between 3rd and 2nd arg. Just having 3rd arg is not enough, you should use the proper one for certain object. Which we check too.

[VincentLanglet]

Do you think this is a good idea @derrabus?

I also need to hear from you how you'd approach this and if it's even possible to detect the right type to use. From your message it's not obvious if you just always want to make the 3rd parameter required, or if you want to detect when the type MUST be used and WHAT type should be used. It'd probably prevent more bugs (but might be hard to do).

Since, @beberlei, you're the author of the issue doctrine/orm#8379 maybe you could also share your point on view on this issue ?

[beberlei]

The performance issue is when passing objects, not other types. A general rule forcing type setting is overkill since there is good auto detection and that shortens code.

[VincentLanglet]

The performance issue is when passing objects, not other types. A general rule forcing type setting is overkill since there is good auto detection and that shortens code.

So an appropriate rule would be the same than psalm/psalm-plugin-symfony#158 which requires to force setting type when we're passing an object (or an array of object ?).

I also need to hear from you how you'd approach this and if it's even possible to detect the right type to use. From your message it's not obvious if you just always want to make the 3rd parameter required, or if you want to detect when the type MUST be used and WHAT type should be used.

@ondrejmirtes As a first step, I'd like to force setting the type without having to detect the one that must be used.

The message on psalm side is

To improve performance set explicit type for objects

Maybe we could improve it to something like "Set explicit type or avoid passing an object", because when an object is managed by ORM, it seems better to pass $foo->getId() rather than $foo as parameter.

[derrabus]

Do you think this is a good idea @derrabus?

I wouldn't force the third parameter because in many cases it's just fine to omit it. But I realize that there's a pitfall there that you're trying to avoid.

A very simple rule could emit an error if the third parameter is omitted for a non-scalar value. If we have more insights on the types registered for the corresponding DBAL connection, we could even emit an error if the parameter value cannot be converted by the specified type. Without that information, we could at least check the value type if the ParameterType and ArrayParameterType enums were used as type specifiers.

[VincentLanglet]

A very simple rule could emit an error if the third parameter is omitted for a non-scalar value.

Yes, that would be great

[janedbal]

A very simple rule could emit an error if the third parameter is omitted for a non-scalar value.

But entity is fine, those should be excluded, right?

[VincentLanglet]

A very simple rule could emit an error if the third parameter is omitted for a non-scalar value.

But entity is fine, those should be excluded, right?

It depends...

It may be argued that passing a entity will add a metadata-load when we could pass directly the identifier.
I assume there is some cache on metadata-load, but we cannot be sure the metadata is already cached.

Seems better to pass $entity->getId() rather than $entity in such situation.

But if we go that way, the same argument could be used for calls like

$repository->findBy(['field' => $object]);

vs

$repository->findBy(['field' => $object->getId()]);

and I dunno if it's the road to take.

[stof]

if your entity was loaded from the database already (which is probably the case if it already has an id), you can be 100% sure that metadata are already loaded for the class (as hydrating an entity requires knowing its metadata)