[Core: Database] Add exception on fetchAll failure case

[johnsaigle]

Brief summary of changes

In an effort to take advantage of PHP's type system, we should add type declarations to our code, especially in the core classes.

The PDO documentation says that fetchAll returns the empty set when no data in the DB matches a given query and FALSE on failure. However we do not distinguish this in our codebase and so a FALSE value is allowed to bubble up through subsequent DB calls.

This makes it so that these functions must sometimes return the Bool type (in addition to String, Array, and Null) meaning we cannot use return type declarations.

Following a discussion in #3854, this PR adds an exception when the fetchAll class fails. This will allow us to remove Bool as one of the return types.

This is likely to break some things (I found it by debugging a failing test in #3878) so it is issued to major.

Related issues

• [Database] Change return type of pselectOne to null #3854
• [Core: Database] Add type declarations to function signatures #3878

[driusan]

this isn't checking if it's false, it's checking if it's a boolean. It should be === false

[johnsaigle]

I tried that at first but phan gave me an error about comparing an array to a boolean (in the case where it was NOT false).

I'll change this statement to include both and see how phan feels about it.

[driusan]

the docs pretty clearly say that the return type is array|false, so if phan is complaining about checking the false case it sounds like a bug in phan.

[xlecours]

How about !is_array() ?

[johnsaigle]

@driusan @xlecours This is ready for re-review.

[xlecours]

I think that this highlights the fact that the Database::execute function is doing more than just one thing.

[johnsaigle]

@xlecours Yeah a lot of the DB functions return many many different types which makes validation and use of them kinda tricky. This is one small step in the process....