Fix wrong types for AbstractQuery and child classes #9774

greg0ire · 2022-05-21T13:26:28Z

greg0ire · 2022-05-21T13:52:47Z

lib/Doctrine/ORM/AbstractQuery.php

-     * @return mixed[]|string|int|float|bool
-     * @psalm-return array|scalar
+     * @return mixed[]|string|int|float|bool|object|null
+     * @psalm-return array|scalar|object|null


I've seen DateTime, CustomIdObject and null pass here when running unit tests with a native type declaration.

That's basically mixed, isn't it?

Oh crap didn't see your answer… well I guess you can't have resource in there but yeah, mixed might be better here 😅

greg0ire · 2022-05-21T13:53:10Z

lib/Doctrine/ORM/NativeQuery.php

@@ -34,7 +34,7 @@ public function setSQL($sql): self
    /**
     * Gets the SQL query.
     *
-     * @return mixed The built SQL query or an array of all SQL queries.
+     * @return string The built SQL query


The class is final, and I don't see how we could have anything else than a string here.

If you're sure about that, you can add a native return type. Doing that on a final class is not a BC break.

greg0ire · 2022-05-21T13:53:47Z

psalm-baseline.xml

@@ -1970,6 +1964,9 @@
    <PropertyNotSetInConstructor occurrences="1">
      <code>MultiTableUpdateExecutor</code>
    </PropertyNotSetInConstructor>
+    <PropertyTypeCoercion occurrences="1">
+      <code>$this-&gt;_sqlStatements</code>
+    </PropertyTypeCoercion>


The code is hard to analyse statically, but from my human point of view, what is intended here is a list

orm/lib/Doctrine/ORM/Query/Exec/MultiTableUpdateExecutor.php

Lines 88 to 124 in 8f77012

$i = -1;

foreach (array_reverse($classNames) as $className) {

$affected = false;

$class = $em->getClassMetadata($className);

$updateSql = 'UPDATE ' . $quoteStrategy->getTableName($class, $platform) . ' SET ';

foreach ($updateItems as $updateItem) {

$field = $updateItem->pathExpression->field;

if (

(isset($class->fieldMappings[$field]) && ! isset($class->fieldMappings[$field]['inherited'])) ||

(isset($class->associationMappings[$field]) && ! isset($class->associationMappings[$field]['inherited']))

) {

$newValue = $updateItem->newValue;

if (! $affected) {

$affected = true;

++$i;

} else {

$updateSql .= ', ';

}

$updateSql .= $sqlWalker->walkUpdateItem($updateItem);

if ($newValue instanceof AST\InputParameter) {

$this->_sqlParameters[$i][] = $newValue->name;

++$this->_numParametersInUpdateClause;

}

}

}

if ($affected) {

$this->_sqlStatements[$i] = $updateSql . ' WHERE (' . $idColumnList . ') IN (' . $idSubselect . ')';

}

}

Agree. mixed[] doesn't fit for _sqlStatements.

SenseException

The issue references 3.0.0. Is 2.12.x the expected target branch?

SenseException · 2022-05-22T22:18:08Z

psalm-baseline.xml

@@ -1970,6 +1964,9 @@
    <PropertyNotSetInConstructor occurrences="1">
      <code>MultiTableUpdateExecutor</code>
    </PropertyNotSetInConstructor>
+    <PropertyTypeCoercion occurrences="1">
+      <code>$this-&gt;_sqlStatements</code>
+    </PropertyTypeCoercion>


Agree. mixed[] doesn't fit for _sqlStatements.

greg0ire · 2022-05-23T05:49:17Z

The issue references 3.0.0. Is 2.12.x the expected target branch?

This PR is linked to 2 other PRs :)

derrabus · 2022-05-23T08:56:07Z

lib/Doctrine/ORM/AbstractQuery.php

@@ -282,7 +283,7 @@ public function setCacheMode($cacheMode)
     * The returned SQL syntax depends on the connection driver that is used
     * by this query object at the time of this method call.
     *
-     * @return string SQL query
+     * @return list<string>|string SQL query


For 3.0, we should try to replace this method with one that always returns an array of strings.

Doctrine noob here: I assume a single string is the SQL query, probably with placeholders and such, but what is an array of strings? And why would we prefer this for Doctrine 3?

derrabus · 2022-05-23T08:59:37Z

lib/Doctrine/ORM/Query.php

@@ -596,7 +596,7 @@ public function free(): void
    /**
     * Sets a DQL query string.
     *
-     * @param string $dqlQuery DQL Query.
+     * @param string|null $dqlQuery DQL Query.


This method is a no-op when null is passed. Should we rather deprecate passing null so that we can use string as native parameter type in 3.0?

Should we rather deprecate passing null so that we can use string as native parameter type in 3.0?

Yes, but not rather. We should do so on 2.13.x.

I do not think we actually want to force our users to build an array collection when they want to use setParameters().

* 2.12.x: Fix wrong types for AbstractQuery and child classes (doctrine#9774) Document callable as possible Add use statement (doctrine#9769)

* 2.13.x: Make phpdoc more precise Deprecate setting fetch mode to random integers Prepare split of output walkers and tree walkers (doctrine#9786) PHPStan 1.7.0 (doctrine#9785) Deprecate passing null to Query::setDQL() Kill call_user_func(_array) (doctrine#9780) Fix wrong types for AbstractQuery and child classes (doctrine#9774) Document callable as possible Remove override phpdoc tag Add use statement (doctrine#9769)

greg0ire force-pushed the fix-wrong-types-abstract-query branch 4 times, most recently from 0bf0e20 to 4a12afa Compare May 21, 2022 13:47

greg0ire commented May 21, 2022

View reviewed changes

greg0ire requested review from derrabus and SenseException May 21, 2022 13:53

greg0ire force-pushed the fix-wrong-types-abstract-query branch from 4a12afa to 920d278 Compare May 22, 2022 14:48

greg0ire changed the title ~~Fix wrong types for AbstractQuery~~ Fix wrong types for AbstractQuery and child classes May 22, 2022

greg0ire mentioned this pull request May 22, 2022

Make phpdoc more precise for AbstractQuery #9777

Merged

SenseException reviewed May 22, 2022

View reviewed changes

derrabus reviewed May 23, 2022

View reviewed changes

Remove comment about BC

92c28ab

I do not think we actually want to force our users to build an array collection when they want to use setParameters().

greg0ire force-pushed the fix-wrong-types-abstract-query branch from 920d278 to eb28264 Compare May 23, 2022 09:03

Make phpdoc more accurate

e9c868d

greg0ire force-pushed the fix-wrong-types-abstract-query branch from eb28264 to e9c868d Compare May 23, 2022 09:05

derrabus approved these changes May 23, 2022

View reviewed changes

derrabus merged commit 1f63389 into doctrine:2.12.x May 23, 2022

derrabus added a commit to derrabus/orm that referenced this pull request May 23, 2022

Merge branch '2.12.x' into 2.13.x

a131878

* 2.12.x: Fix wrong types for AbstractQuery and child classes (doctrine#9774) Document callable as possible Add use statement (doctrine#9769)

greg0ire deleted the fix-wrong-types-abstract-query branch May 23, 2022 09:50

greg0ire mentioned this pull request Jun 1, 2022

Migrate to PHP 8.1 #9772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wrong types for AbstractQuery and child classes #9774

Fix wrong types for AbstractQuery and child classes #9774

greg0ire commented May 21, 2022

greg0ire May 21, 2022

derrabus May 23, 2022

greg0ire May 23, 2022

greg0ire May 21, 2022

derrabus May 23, 2022

greg0ire May 21, 2022 •

edited

Loading

SenseException May 22, 2022

SenseException left a comment

SenseException May 22, 2022

greg0ire commented May 23, 2022

derrabus May 23, 2022

morrisonlevi Sep 2, 2022

derrabus May 23, 2022

greg0ire May 23, 2022

	$i = -1;

	foreach (array_reverse($classNames) as $className) {
	$affected = false;
	$class = $em->getClassMetadata($className);
	$updateSql = 'UPDATE ' . $quoteStrategy->getTableName($class, $platform) . ' SET ';

	foreach ($updateItems as $updateItem) {
	$field = $updateItem->pathExpression->field;

	if (
	(isset($class->fieldMappings[$field]) && ! isset($class->fieldMappings[$field]['inherited'])) \|\|
	(isset($class->associationMappings[$field]) && ! isset($class->associationMappings[$field]['inherited']))
	) {
	$newValue = $updateItem->newValue;

	if (! $affected) {
	$affected = true;
	++$i;
	} else {
	$updateSql .= ', ';
	}

	$updateSql .= $sqlWalker->walkUpdateItem($updateItem);

	if ($newValue instanceof AST\InputParameter) {
	$this->_sqlParameters[$i][] = $newValue->name;

	++$this->_numParametersInUpdateClause;
	}
	}
	}

	if ($affected) {
	$this->_sqlStatements[$i] = $updateSql . ' WHERE (' . $idColumnList . ') IN (' . $idSubselect . ')';
	}
	}

Fix wrong types for AbstractQuery and child classes #9774

Fix wrong types for AbstractQuery and child classes #9774

Conversation

greg0ire commented May 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire May 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SenseException left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire May 21, 2022 •

edited

Loading