Stubbing Query class

[dmecke]

I need a lot of suppressions because the methods of the Query class like getResult(), getSingleResult(), getOneOrNullResult(), etc. return mixed. I've tried to create a stub for the AbstractQuery (thats where the methods actually live), but I was only able to let it return array (in the case of getResult()) which causes other issues. I am not really familiar with the concepts of generics, but I think this would be the way to go here.

Any hints whether this is possible at all and maybe point me in the right direction?

[weirdan]

I knew this would come up sooner or later 😆.

Long story short, implementing this with any degree of robustness would require creating mini-Psalm for DQL queries. Not entirely impossible, but certainly not the easiest thing to do. Not really possible with simple approach like stubbing/generics.

Generics operate on types, and all you've got, type-wise, is string. Query result, however, depends on the string contents, which might not even be known upfront. Consider the following code:

$result = $em->createQuery(file_get_contents('somefile.dql'))->getResult();

Here, static analysis tool like Psalm has no way to know if that was SELECT, DELETE or UPDATE query, what fields there were mentioned, what were FROM clauses, etc. All of those factors affect the result set shape.

If we consider a more restricted case of a known literal string, that could be parsed to figure out aforementioned factors. Would require custom inference code (probably reusing Doctrine query parser).

To use Doctrine parser we would need to find a way to obtain initialized and configured EntityManager, which means the plugin would need fully bootstrapped Doctrine environment, configured the same way you do in your application.

Having parsed AST\*Statement plugin would have to analyze it, correlate the results with what Psalm knows about entity classes and produce the final output type.

On the upside, it would be able to report issues in DQL queries.

[weirdan]

What you could do instead is to specify the result type manually like this:

        /** @var BlogPost[] $result */
        $result = $query->getResult();

Of course it becomes your responsibility to make sure the type actually matches that getResult() would return.

[dmecke]

Thank you very much for the detailed explanation, it is very appreciated!

Ok, I understand that the safe solution is more a project in itself. Your suggestions is what also came to my mind and maybe a good middleground. But would it be possible to use instanceof checks instead of the inline docblock which is, as you said, very fragile? Something like this:

$result = $query->getResult();
foreach ($result as $element) {
    if (!$element instanceof BlogPost) {
        throw new InvalidTypeException();
    }
}

Does this proof psalm that all elements are of the type BlogPost? If that's the case this should be more safe as the docblock as the code does not rot. But as $query->getResult() still returns mixed it also requires as stub that at least ensures that $result is iterable.. Hm...

[weirdan]

No, foreach wouldn't work like that (see vimeo/psalm#649), but I have even better idea: https://getpsalm.org/r/7396e44181
It doesn't work right now either, though. I've reported the issue here: vimeo/psalm#1227. Fingers crossed it'll get fixed soon.

It completely avoids runtime overhead when zend.assertions php ini setting is set to -1 (production value). In dev and CI you would have it set to 1, so that checks are executed.

Keep in mind that it would only work with a rewindable iterables. Non-rewindable iterables (like generators) will have become consumed by the time allInstanceOf returns - but so they would be with foreach, so it shouldn't be much of a problem.

[dmecke]

Thanks a lot for the effort you put into this.

My problem with the assert approach is that it does not check the types during runtime which, on the one hand is good performance-wise as you pointed out, but on the other hand lets slip through cases that occur in the context of the application (maybe for some reason there is a null in your list of BlogPost). But eventually this is a balance between performance and type safety and strongly depends on the needs of the specific application / use case.

[weirdan]

Requires #10

[vv12131415]

Maybe it will make our lives easier with this info that will be easier to implement

the return type of getResult depends on the first parameter
T - is the entity related to this repository
HYDRATE_OBJECT - list<T|array<array-key,mixed>>
the name of the constant is misleading,
list<array<array-key,mixed>> will be returned if I do SELECT (user.name) FROM Namespace\User user it returns something like this

[
  0 => [
    1 => "asddas 2"
  ]
  1 => [
    1 => "asddas"
  ]
]

this might need to be reported to Doctrine, so that HYDRATE_OBJECT hydrates only objects, and if it can't it will fail
HYDRATE_ARRAY - list<array<array-key,mixed>>
HYDRATE_SCALAR - list<array<array-key,mixed>> (same as HYDRATE_ARRAY)
HYDRATE_SINGLE_SCALAR - scalar
HYDRATE_SIMPLEOBJECT - list<T>

return types for getSingleResult
HYDRATE_OBJECT - T|array<array-key,mixed>
HYDRATE_ARRAY - array<array-key,mixed>
HYDRATE_SCALAR - array<array-key,mixed> (same as HYDRATE_ARRAY)
HYDRATE_SINGLE_SCALAR - scalar
HYDRATE_SIMPLEOBJECT - T

getArrayResult returns list<array<array-key,mixed>> (same as getResult(AbstractQuery::HYDRATE_ARRAY))
getScalarResult returnslist<array<array-key,mixed>> (same as getResult(AbstractQuery::HYDRATE_SCALAR))

getOneOrNullResult returns
HYDRATE_OBJECT - T|array<array-key,mixed>|null
HYDRATE_ARRAY - array<array-key,mixed>|null
HYDRATE_SCALAR - array<array-key,mixed>|null (same as HYDRATE_ARRAY)
HYDRATE_SINGLE_SCALAR - scalar
HYDRATE_SIMPLEOBJECT - T|null