Migrate to PHP 8: AbstractQuery and child classes #9776

greg0ire · 2022-05-22T14:50:45Z

Blocked by

TODO:

merge the above
merge up the patch branch
merge up the minor branch
run rector

lib/Doctrine/ORM/AbstractQuery.php

derrabus · 2022-05-25T12:40:39Z

lib/Doctrine/ORM/AbstractQuery.php

@@ -217,59 +196,52 @@ public function setCacheRegion($cacheRegion)
     *
     * @return string|null The cache region name; NULL indicates the default region.


Suggested change

* @return string|null The cache region name; NULL indicates the default region.

* Returns the cache region name or NULL to indicate the default region.

lib/Doctrine/ORM/AbstractQuery.php

derrabus · 2022-05-25T12:47:48Z

lib/Doctrine/ORM/AbstractQuery.php

@@ -348,7 +315,7 @@ static function (Query\Parameter $parameter) use ($key): bool {
     *
     * @return $this
     */
-    public function setParameters($parameters)
+    public function setParameters(ArrayCollection|array $parameters): static


We should try to narrow this down in 2.13.

🤔 what do you mean? Drop the ArrayCollection| part? I know I have never used it that way.

derrabus · 2022-05-25T12:50:25Z

lib/Doctrine/ORM/AbstractQuery.php

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


Do these annotations provide any real value? They're hardly more specifiy than mixed.

I'll try widening to mixed on 2.13.x, see if that impacts static analysis.

lib/Doctrine/ORM/AbstractQuery.php

derrabus · 2022-05-25T12:55:48Z

lib/Doctrine/ORM/AbstractQuery.php

@@ -949,14 +870,12 @@ public function getHints()
     * Executes the query and returns an iterable that can be used to incrementally
     * iterate over the result.
     *
-     * @param ArrayCollection|array|mixed[] $parameters    The query parameters.
-     * @param string|int|null               $hydrationMode The hydration mode to use.
     * @psalm-param ArrayCollection<int, Parameter>|mixed[] $parameters


It's a bit odd that we use iterable as native type, but a more specific ArrayCollection as annotated type. I think, we should fix the annotation.

I agree it's a problem, but I don't think fixing the annotation is the right thing to do: down the line, we call setParameters, which can deal with mixed[], but not with ArrayCollection<stdClass>. It needs to be ArrayCollection<Parameter>.

You're right. But then, we should have the same native parameter type as setParameters(), so iterable should be ArrayCollection|array.

Yes, definitely, I can change that in this PR 👍

greg0ire · 2022-05-25T14:26:31Z

@derrabus I'm AFK until Sunday, feel free to push to my branch, I agree with all of your comments.

derrabus · 2022-06-01T12:33:09Z

lib/Doctrine/ORM/Query.php

@@ -479,16 +451,12 @@ public function useQueryCache($bool): self
    /**
     * Defines how long the query cache will be active before expire.
     *
-     * @param int $timeToLive How long the cache entry is valid.
+     * @param int|null $timeToLive How long the cache entry is valid.


Should be fixed on 2.13.x as well.

"fixed"? 2.12.x then.

derrabus · 2022-06-01T12:35:07Z

lib/Doctrine/ORM/Query.php

@@ -509,7 +477,7 @@ public function getQueryCacheLifetime(): ?int
     *
     * @return $this
     */
-    public function expireQueryCache($expire = true): self
+    public function expireQueryCache(bool $expire = true): self


We can delete the corresponding @param, I think.

derrabus · 2022-06-01T12:35:21Z

lib/Doctrine/ORM/Query.php

     *
     * @param string|null $dqlQuery DQL Query.


Suggested change

*

* @param string|null $dqlQuery DQL Query.

derrabus · 2022-06-01T12:36:14Z

lib/Doctrine/ORM/Query.php

@@ -597,7 +565,7 @@ public function contains($dql): bool
     *
     * @return $this
     */
-    public function setFirstResult($firstResult): self
+    public function setFirstResult(?int $firstResult): self


Shall we disallow null, to be consistent with the DBAL?

Yes, that would make sense. That will need to be initiated on 2.13.x

greg0ire · 2022-06-02T21:26:57Z

lib/Doctrine/ORM/AbstractQuery.php

-            );
-            $fetchMode = Mapping\ClassMetadata::FETCH_LAZY;
-        }
-


From #9777 (comment) :

On 3.0, we would throw if the caller passed anything but the two allowed values, right?

Do we really want to do this? If yes, maybe we should require an assertion library 🤔

derrabus · 2022-06-03T11:14:00Z

lib/Doctrine/ORM/AbstractQuery.php

+        /**
+         * The entity manager used by this query object.
+         */
+        protected EntityManagerInterface $_em


I'd rather rename the property to em than adding that _ prefix to the constructor parameter, tbh.

Me too, that was me not questioning Rector's work.

derrabus · 2022-06-03T11:18:55Z

lib/Doctrine/ORM/AbstractQuery.php


        $filteredParameters = $this->parameters->filter(
-            static function (Query\Parameter $parameter) use ($key): bool {
+            static function (Parameter $parameter) use ($key): bool {


$filteredParameters = $this->parameters->filter( static fn (Parameter $parameter): bool => $parameter->getName() === $key );

Both array and ArrayCollection are countable.

greg0ire force-pushed the native-types-abstract-query branch from 2ba2640 to 55c87e2 Compare May 22, 2022 14:53

derrabus reviewed May 24, 2022

View reviewed changes

lib/Doctrine/ORM/AbstractQuery.php Outdated Show resolved Hide resolved

lib/Doctrine/ORM/AbstractQuery.php Outdated Show resolved Hide resolved

lib/Doctrine/ORM/AbstractQuery.php Outdated Show resolved Hide resolved

greg0ire force-pushed the native-types-abstract-query branch 4 times, most recently from 2a19761 to a492e0a Compare May 25, 2022 06:46

greg0ire marked this pull request as ready for review May 25, 2022 06:48

greg0ire requested review from derrabus and SenseException May 25, 2022 06:48

derrabus requested changes May 25, 2022

View reviewed changes

greg0ire force-pushed the native-types-abstract-query branch from 174dd98 to 124073d Compare May 31, 2022 11:55

SenseException previously approved these changes May 31, 2022

View reviewed changes

derrabus reviewed Jun 1, 2022

View reviewed changes

This was referenced Jun 1, 2022

Migrate to PHP 8.1 #9772

Closed

Fix wrong types #9802

Merged

greg0ire dismissed SenseException’s stale review via 64ee704 June 1, 2022 21:07

greg0ire force-pushed the native-types-abstract-query branch from 124073d to 64ee704 Compare June 1, 2022 21:07

greg0ire requested a review from derrabus June 1, 2022 21:15

greg0ire force-pushed the native-types-abstract-query branch from 64ee704 to 8399c8c Compare June 2, 2022 20:56

greg0ire marked this pull request as draft June 2, 2022 20:57

greg0ire changed the title ~~Add native types to AbstractQuery~~ Migrate to PHP 8: AbstractQuery and child classes Jun 2, 2022

greg0ire force-pushed the native-types-abstract-query branch from 5d03a72 to 8e05c1c Compare June 2, 2022 21:22

greg0ire commented Jun 2, 2022

View reviewed changes

derrabus reviewed Jun 3, 2022

View reviewed changes

Add native types to AbstractQuery

86d23f4

greg0ire force-pushed the native-types-abstract-query branch from 8e05c1c to 740ed9d Compare June 3, 2022 18:03

greg0ire requested review from derrabus and SenseException June 3, 2022 18:03

greg0ire marked this pull request as ready for review June 3, 2022 18:03

Run rector

9f4339e

greg0ire force-pushed the native-types-abstract-query branch from 740ed9d to 205d179 Compare June 3, 2022 18:06

derrabus previously approved these changes Jun 3, 2022

View reviewed changes

greg0ire added 3 commits June 3, 2022 20:51

Remove deprecation when wrong fetch mode is used

2ee4482

Remove unneeded checks

4a02a79

Both array and ArrayCollection are countable.

Update baseline

d462ed3

greg0ire dismissed derrabus’s stale review via d462ed3 June 3, 2022 18:51

greg0ire force-pushed the native-types-abstract-query branch from 205d179 to d462ed3 Compare June 3, 2022 18:51

greg0ire merged commit 53661fe into doctrine:3.0.x Jun 3, 2022

greg0ire deleted the native-types-abstract-query branch June 3, 2022 18:53

greg0ire added this to the 3.0.0 milestone Jun 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to PHP 8: AbstractQuery and child classes #9776

Migrate to PHP 8: AbstractQuery and child classes #9776

greg0ire commented May 22, 2022 •

edited

Loading

derrabus May 25, 2022

derrabus May 25, 2022

greg0ire May 31, 2022

derrabus May 25, 2022

greg0ire May 31, 2022

greg0ire Jun 1, 2022

derrabus May 25, 2022

greg0ire May 31, 2022

derrabus Jun 1, 2022

greg0ire Jun 1, 2022

greg0ire commented May 25, 2022

derrabus Jun 1, 2022

greg0ire Jun 1, 2022

derrabus Jun 1, 2022

derrabus Jun 1, 2022

derrabus Jun 1, 2022

greg0ire Jun 1, 2022

greg0ire Jun 2, 2022

derrabus Jun 3, 2022

greg0ire Jun 3, 2022

derrabus Jun 3, 2022

		@@ -217,59 +196,52 @@ public function setCacheRegion($cacheRegion)
		*
		* @return string\|null The cache region name; NULL indicates the default region.

	* @return string\|null The cache region name; NULL indicates the default region.
	* Returns the cache region name or NULL to indicate the default region.

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

Migrate to PHP 8: AbstractQuery and child classes #9776

Migrate to PHP 8: AbstractQuery and child classes #9776

Conversation

greg0ire commented May 22, 2022 • edited Loading

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

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 commented May 25, 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

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 commented May 22, 2022 •

edited

Loading