Use better typing in QueryBuilder #4220
Conversation
greg0ire
force-pushed
the
describe-sql-parts
branch
from
88956cc to
e9f7f69
Compare
|
@morozov while working on this, I noticed that upgrading Psalm produced many errors related to the named parameters RFC, because of changes in parameter names between abstractions and concretions. I could have sworn you already worked on this, but could not find anything. Can you point me to a PR where you worked on this? We might want to backport it. |
| @@ -414,6 +430,8 @@ public function getMaxResults() | |||
| * @param bool $append | |||
| * | |||
| * @return $this This QueryBuilder instance. | |||
| * | |||
| * @psalm-param 'select'|'from'|'set'|'where'|'groupBy'|'having'|'orderBy' | |||
There was a problem hiding this comment.
Good catch, this reveals I missed join and values
| @@ -1273,7 +1291,7 @@ public function __toString() | |||
| * @link http://www.zetacomponents.org | |||
| * | |||
| * @param mixed $value | |||
| * @param mixed $type | |||
| * @param int $type | |||
There was a problem hiding this comment.
Dbal supports to use string as bind type, isn't it?
There was a problem hiding this comment.
How so? You mean by using string instead of ParameterType::STRING, which is an integer?
There was a problem hiding this comment.
It should be compatible with:
dbal/lib/Doctrine/DBAL/Statement.php
Line 87 in 1f84cc8
which is also not annotated properly. It's string|int|Type|null (see other branches).
There was a problem hiding this comment.
Ok, I annotated both (and more) as you described.
It makes it easier for Psalm (and arguably for humans) to understand this piece of code.
greg0ire
force-pushed
the
describe-sql-parts
branch
from
e9f7f69 to
9936a2a
Compare
|
Ok, I thought it was on higher branches but no, it's just that there are other signatures that are affected by this. I created #4221 to keep track of that |
greg0ire
force-pushed
the
describe-sql-parts
branch
from
9936a2a to
1ebaa93
Compare
greg0ire
force-pushed
the
describe-sql-parts
branch
from
1ebaa93 to
8514fbb
Compare
| @@ -187,6 +202,8 @@ public function getConnection() | |||
| * Gets the state of this query builder instance. | |||
| * | |||
| * @return int Either QueryBuilder::STATE_DIRTY or QueryBuilder::STATE_CLEAN. | |||
| * | |||
| * @psalm-return QueryBuilder::STATE_* | |||
There was a problem hiding this comment.
Why is this different from the type of $state?
| * @param string $placeHolder The name to bind with. The string must start with a colon ':'. | ||
| * @param mixed $value | ||
| * @param string|int|Type|null $type | ||
| * @param string $placeHolder The name to bind with. The string must start with a colon ':'. |
|
Given that none of the issues found during the review were found by any of the static analysis tools, what's the point of this improvement at this moment? Is it a pre-condition for say Psalm update? |
greg0ire
force-pushed
the
describe-sql-parts
branch
from
8514fbb to
51bfc87
Compare
Not at all, what prompted me to do this is #4219 , where I noticed that the code was hard to understand because of all the possibilities of the structure in |
|
Ok, I'll close this, it didn't turn out as good as I hoped anyway. |
Summary
That class is a bit hard to understand, so I tried to be as precise as possible while keeping both Psalm and Phpstan happy (which means I couldn't be as precise as I wanted).