Strict typing in Doctrine\ORM, disconnect Doctrine\Common by Majkl578 · Pull Request #7130 · doctrine/orm

Majkl578 · 2018-03-14T04:36:31Z

Initial round of strict typing.

Disconnected Doctrine\Common persistence for now to allow typehinting changes and Doctrine\Persistence migration.
Typehints for classes in the root Doctrine\ORM namespace.
Conflict to prophecy versions without PHP 7.2 support (to be dropped in future).

TODO

document signature changes - @Ocramius can you please suggest some format? Maybe outside UPGRADE to keep it clean, i.e. something like UPGRADE-STRICT-API.md with diff-like list of changes.
squash?

SenseException · 2018-03-14T09:22:06Z

tests/Doctrine/Performance/Mock/NonProxyLoadingEntityManager.php

     * {@inheritDoc}
     */
-    public function createQuery($dql = '') : Query
+    public function createQuery(string $dql = '') : Query


While introducing BC breaks for the next major release, should we keep the default empty string? Does it make sense to have that? Or is this part of a later discussion?

Adding parameter type is not a BC break.
At this time I don't want to change logic or accepted types too much where not needed, otherwise the PR(s) would be pain to review. That's also why I i.e. didn't improve typing for QueryBuilder and parameter binding/types in general.

Oh, right. Type widening in PHP 7.2. PHP 7.2 will be minimum version. I totally forgot that.

Taluu · 2018-03-14T09:30:14Z

composer.json

    },
+    "conflict": {
+        "phpspec/prophecy": "<1.7.5"
+    },


You are blocking potential users of prophecy < 1.7.5 even if they don't intend to test doctrine

So instead of how requiring it in the require-dev section ?

Technically legit, but no one is supposed to use dev-master now so it's not a real issue. This is mostly to make CI work with lowest dependencies as well (too bad there is no conflict-dev section).

It doesn't make any sense to have a conflict-dev, as if you install the dev dependencies, it means you cloned the doctrine repo and working directly on it (you can't install dev dependencies of a dependency...). So making it into the dev section of the requirement and removing the conflict makes sense IMO.

So instead of how requiring it in the require-dev section ?

Nope, let's please not do that.

Users will be perfectly fine as long as they upgrade all their dependencies as they should be doing

ostrolucky

Be careful about $hydrationMode, it can be a string for custom hydrators, see here. Yes, int in docblock was wrong all the time.

I've not highlighted all references, it became too much and would just clutter this PR

ostrolucky · 2018-03-17T14:33:51Z

lib/Doctrine/ORM/AbstractQuery.php

     * @return mixed
     */
-    private function executeIgnoreQueryCache($parameters = null, $hydrationMode = null)
+    private function executeIgnoreQueryCache(?iterable $parameters = null, ?int $hydrationMode = null)


$hydrationMode can be string for custom hydrators

ostrolucky · 2018-03-17T14:34:40Z

lib/Doctrine/ORM/AbstractQuery.php

     * @return mixed
     */
-    public function getResult($hydrationMode = self::HYDRATE_OBJECT)
+    public function getResult(int $hydrationMode = self::HYDRATE_OBJECT)


$hydrationMode can be string for custom hydrators

ostrolucky · 2018-03-17T14:35:06Z

lib/Doctrine/ORM/AbstractQuery.php

     * @return mixed
     */
-    public function execute($parameters = null, $hydrationMode = null)
+    public function execute(?iterable $parameters = null, ?int $hydrationMode = null)


$hydrationMode can be string for custom hydrators

ostrolucky · 2018-03-17T14:35:37Z

lib/Doctrine/ORM/AbstractQuery.php

     * @return mixed
     */
-    private function executeUsingQueryCache($parameters = null, $hydrationMode = null)
+    private function executeUsingQueryCache(?iterable $parameters = null, ?int $hydrationMode = null)


$hydrationMode can be string for custom hydrators

…ectable

…t}Collection

Majkl578 · 2018-03-31T16:15:25Z

@ostrolucky Thanks, adressed. Probably something to look into later to remove int|string union.

ostrolucky · 2018-04-02T00:28:57Z

lib/Doctrine/ORM/Event/LifecycleEventArgs.php

-     *
-     * @return object
     */
    public function getEntity()


You removed return type annotation without adding return type. Same for following methods in this class

Ocramius · 2018-04-02T10:10:15Z

document signature changes - @Ocramius can you please suggest some format? Maybe outside UPGRADE to keep it clean, i.e. something like UPGRADE-STRICT-API.md with diff-like list of changes.

No idea how to make this clearer for readers. UPGRADE.md is the location read by people upgrading the lib: why would this be documented outside it?

Ocramius · 2018-04-13T15:37:54Z

document signature changes - @Ocramius can you please suggest some format? Maybe outside UPGRADE to keep it clean, i.e. something like UPGRADE-STRICT-API.md with diff-like list of changes.

No idea how to make this clearer for readers. UPGRADE.md is the location read by people upgrading the lib: why would this be documented outside it?

Errata: I got a better idea here.

Let's try this upgrade path:

all touched files must become either interface or final class - nothing in-between except for known edge-cases (EntityManager?)
let's list all classes where final was added
let's list all the interfaces in the diff

That's a sufficient UPGRADE.md entry.

Ocramius · 2018-04-13T15:39:47Z

Also: I'm working on roave/api-compare this weekend: hope that will help in automating things. This patch is a good test-bed for that library too 👍 /cc @asgrim

Majkl578 added WIP Improvement BC Break labels Mar 14, 2018

Majkl578 added this to the 3.0 milestone Mar 14, 2018

Majkl578 requested a review from Ocramius March 14, 2018 04:36

Majkl578 force-pushed the dev/strict-types branch 3 times, most recently from fd6c6c9 to ac31815 Compare March 14, 2018 05:01

SenseException reviewed Mar 14, 2018

View reviewed changes

Taluu reviewed Mar 14, 2018

View reviewed changes

ostrolucky requested changes Mar 17, 2018

View reviewed changes

Majkl578 mentioned this pull request Mar 25, 2018

Removing the metadata instantiation from the EM #7143

Closed

4 tasks

Majkl578 added 13 commits March 31, 2018 18:03

Temporarily disconnect EntityManagerInterface from ObjectManager

3e9f5bf

Temporarily disconnect EntityRepository from ObjectRepository and Sel…

509a2be

…ectable

phpcs.xml.dist: Selective excludes instead of global exclude

d805104

Break PHPSpec version incompatible with PHP 7.2

55029bf

[Strict] Add native typehints to Doctrine\ORM\EntityManager{,Interface}

86be156

[Strict] Add native typehints to Doctrine\ORM\UnitOfWork

dc107cf

[Strict] Add native typehints to Doctrine\ORM\EntityRepository

719ba27

[Strict] Add native typehints to Doctrine\ORM\{Abstract,Native,}Query

65d30fb

[Strict] Add native typehints to Doctrine\ORM\QueryBuilder

903f924

[Strict] Add native typehints to Doctrine\ORM\Cache{,\DefaultCache}

680f366

[Strict] Add native typehints to Doctrine\ORM\{LazyCriteria,Persisten…

fe38c8d

…t}Collection

[Strict] Add native typehints to Doctrine\ORM\PersistentObject

6d171d7

[Strict] Add native typehints to Doctrine\ORM\Configuration

e004a4e

Majkl578 force-pushed the dev/strict-types branch from ac31815 to e004a4e Compare March 31, 2018 16:14

ostrolucky reviewed Apr 2, 2018

View reviewed changes

Majkl578 mentioned this pull request Apr 17, 2018

Inherit from homonym exception in parent package #7199

Merged

Majkl578 closed this Apr 16, 2019

Majkl578 deleted the dev/strict-types branch April 16, 2019 19:51

ostrolucky mentioned this pull request Apr 18, 2019

Add types to all interfaces. doctrine/persistence#50

Merged

greg0ire removed this from the 3.0.0 milestone Jun 27, 2021

Conversation

Majkl578 commented Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Majkl578 Mar 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ostrolucky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Majkl578 commented Mar 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ocramius commented Apr 2, 2018

Uh oh!

Ocramius commented Apr 13, 2018

Uh oh!

Ocramius commented Apr 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Majkl578 commented Mar 14, 2018 •

edited

Loading

Majkl578 Mar 14, 2018 •

edited

Loading