Add type declarations where backwards compatible #1122

greg0ire · 2020-01-06T22:01:26Z

That means:

methods in final classes
final methods (none of those found)
private methods
methods in Tests (that excludes setUp(), tearDown() and test*, which
are handled with an automated tool in Add void return type declaration to tests #1121 )

ostrolucky

It looks good, but needs rebase

ostrolucky · 2020-01-07T20:44:24Z

Controller/ProfilerController.php

@@ -74,7 +74,10 @@ public function explainAction($token, $connectionName, $query)
        ]));
    }

-    private function explainSQLitePlatform(Connection $connection, $query)
+    /**
+     * @param mixed[] $query


This doesn't look useful

ostrolucky · 2020-01-07T20:47:45Z

ManagerConfigurator.php

-     * @param string[] $enabledFilters
-     * @param string[] $filtersParameters
+     * @param string[]                    $enabledFilters
+     * @param array<array<string,string>> $filtersParameters


Suggested change

* @param array<array<string,string>> $filtersParameters

* @param array<string,array<string,string>> $filtersParameters

ostrolucky · 2020-01-07T20:53:23Z

Tests/Command/DropDatabaseDoctrineTest.php

     */
-    private function getMockContainer($connectionName, $params = null)
+    private function getMockContainer(string $connectionName, $params = null) : MockObject


Suggested change

private function getMockContainer(string $connectionName, $params = null) : MockObject

private function getMockContainer(string $connectionName, array $params) : MockObject

ostrolucky · 2020-01-07T20:54:19Z

Tests/Command/CreateDatabaseDoctrineTest.php

     */
-    private function getMockContainer($connectionName, $params = null)
+    private function getMockContainer(string $connectionName, $params = null) : MockObject


Suggested change

private function getMockContainer(string $connectionName, $params = null) : MockObject

private function getMockContainer(string $connectionName, array $params) : MockObject

That means: - methods in final classes - final methods (none of those found) - private methods - methods in Tests (that excludes setUp(), tearDown() and test*, which are handled with an automated tool in a separate commit)

greg0ire · 2020-01-07T21:35:07Z

@ostrolucky here is why we need to use mixed[]: https://travis-ci.org/doctrine/DoctrineBundle/jobs/633955021#L328-L33

I initially did it to convey the fact that I did my due diligence and looked into the type of the items only to find it's mixed instead of just being lazy and use array.

alcaeus · 2020-01-08T09:27:17Z

Please don't target stable releases with such changes - only bug fixes should hit a stable branch with all other changes going to master.

greg0ire · 2020-01-08T16:57:54Z

Sorry, my bad!

ostrolucky · 2020-01-08T17:08:21Z

These are not features either though. It would make solving conflicts much easier to target lowest affected branch.

greg0ire · 2020-01-08T18:16:14Z

It would make solving conflicts much easier to target lowest affected branch.

That's a good point, and IMO it would be more coherent with our stance on docs fixes that don't affect the software either but that should be contributed to the lowest affected branch.

alcaeus · 2020-01-10T09:18:01Z

I'm not going to revert it, but we may want to limit such changes to upcoming releases to avoid issues in patch releases.

ostrolucky · 2020-01-10T09:25:04Z

I think these are very safe changes, I don't see any way to break anything

greg0ire · 2020-01-10T11:07:16Z

I'm torn: on one hand it will make things more complicated for us with the upmerges, especially for that kind of change that touch a lot of files, on the other hand it's hard to say that every pedantic change will always be risk-free, and it's equally hard to ask maintainers to use their best judgment and handle this on a case by case basis. Maybe we should try it for a while and see if it does result in an instability that we are not willing to accept, but maybe some of you already did that in the past, I don't know.

alcaeus · 2020-01-10T11:16:11Z

I've generally avoided touching the type system in patch releases since the breaks can be very subtle, even for private methods. Think about @param string $foo vs. string $foo - these behave very differently with stringables when strict_types is enabled. Introducing such breaks in patch releases is unnecessary, which is why I normally avoid it.

Majkl578 · 2020-03-19T12:54:53Z

Repository/ContainerRepositoryFactory.php

@@ -39,7 +39,7 @@ public function __construct(ContainerInterface $container = null)
    /**
     * {@inheritdoc}
     */
-    public function getRepository(EntityManagerInterface $entityManager, $entityName)
+    public function getRepository(EntityManagerInterface $entityManager, $entityName) : ObjectRepository


This change is in fact a BC break because before repositories were not enforced to be subclass of ObjectRepository. Now they have to be otherwise this will crash on TypeError.

But ContainerRepositoryFactory is final and users can't extends from it. Isn't it?

But ContainerRepositoryFactory is final and users can't extends from it. Isn't it?

Precisely.

@andrew-demb Yes but in Symfony ContainerRepositoryFactory overrides default RepositoryFactory from Doctrine, making it a BC break. Not a BC break in the contract of this class but a BC break in the bundle itself. It's no longer possible to use custom repositories not extending ObjectRepository and doing so makes the app crash due to this added typehint.

It's been documented as supposed to be ObjectRepository since the creation of RepositoryFactory 7 years ago (!) : doctrine/orm@7eb7441

repositories were not enforced to be subclass of ObjectRepository because type declarations did not exist back then, but even if the contract is not enforced, it's still a contract. Call me heartless, but I can't say I feel sorry for apps experiencing this crash, they kind of had it coming, don't you agree?

I don't agree, this kind of stuff does not belong to a patch version.

Requirement for the ObjectRepository/EntityRepository was never documented:

https://github.com/doctrine/orm/blob/v2.7.1/docs/en/reference/working-with-objects.rst#custom-repositories

It was also never validated:

https://github.com/doctrine/orm/blob/v2.7.1/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L2629

https://github.com/doctrine/orm/blob/v2.7.1/lib/Doctrine/ORM/Repository/DefaultRepositoryFactory.php#L68

There is return annotation for EntityManager::getRepository():

https://github.com/doctrine/orm/blob/v2.6.0/lib/Doctrine/ORM/EntityManager.php#L712
but that is the only public API and has never been enforced anyway.

It's not nice "feature" and I don't like it either but it was working and documented behaviour ("You can overwrite this behaviour by specifying the class name of your own Entity Repository in the Annotation, XML or YAML metadata.").
With the above said it rather looks like getRepository()'s @return is incorrect, it's the only place that goes against the actual code.

I don't personally need this "feature" but I don't think this should've been merged as is.

I don't personally need this "feature" but I don't think this should've been merged as is.

You are, as always, entitled to your opinion. To look at the facts:
ContainerRepositoryFactory has always implemented RepositoryFactory from ORM with an @inheritdoc docblock.
RepositoryFactory in ORM has had an @return ObjectRepository on its getRepository method since this commit in 2013, where it was widened from ORM's EntityRepository to the persistence library's ObjectRepository: doctrine/orm@3488049.

I don't know where you see the BC break, but if we treat docblocks as a contract (which we always have and always should), this change is perfectly fine. You are always welcome to improve documentation which is clearly wrong in this case, as the code contains the absolute truth, which is that the contract always specified an ObjectRepository return type for as long as the class has existed.

I don't know where you see the BC break

The BC break I see is that upgrading doctrine/doctrine-bundle from 2.0.6 to 2.0.7 leads to fatal errors.

as the code contains the absolute truth

The code can actually contain bugs.

The BC break I see is that upgrading doctrine/doctrine-bundle from 2.0.6 to 2.0.7 leads to fatal errors.

Please post an example that passes on 2.0.6 and produces a fatal error on 2.0.7 and I'll fix it. Thanks!

greg0ire force-pushed the type-declarations branch from c881cec to 1c8a966 Compare January 6, 2020 22:08

ostrolucky reviewed Jan 7, 2020

View reviewed changes

greg0ire force-pushed the type-declarations branch from 1c8a966 to ee003f1 Compare January 7, 2020 20:41

ostrolucky requested changes Jan 7, 2020

View reviewed changes

greg0ire force-pushed the type-declarations branch from ee003f1 to 1fabf69 Compare January 7, 2020 21:06

Add type declarations where backwards compatible

f7f2f7a

That means: - methods in final classes - final methods (none of those found) - private methods - methods in Tests (that excludes setUp(), tearDown() and test*, which are handled with an automated tool in a separate commit)

greg0ire force-pushed the type-declarations branch from 1fabf69 to f7f2f7a Compare January 7, 2020 21:33

ostrolucky approved these changes Jan 7, 2020

View reviewed changes

ostrolucky merged commit e52fb91 into doctrine:2.0.x Jan 7, 2020

greg0ire deleted the type-declarations branch January 8, 2020 06:49

alcaeus mentioned this pull request Jan 8, 2020

Add void return type declaration to tests #1121

Merged

alcaeus added this to the 2.0.7 milestone Jan 18, 2020

alcaeus added the Improvement label Jan 18, 2020

doctrinebot mentioned this pull request Jan 18, 2020

Merge release 2.0.7 into master #1127

Merged

Majkl578 reviewed Mar 19, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type declarations where backwards compatible #1122

Add type declarations where backwards compatible #1122

greg0ire commented Jan 6, 2020 •

edited

Loading

ostrolucky left a comment

ostrolucky Jan 7, 2020

ostrolucky Jan 7, 2020

ostrolucky Jan 7, 2020

ostrolucky Jan 7, 2020

greg0ire commented Jan 7, 2020

alcaeus commented Jan 8, 2020

greg0ire commented Jan 8, 2020

ostrolucky commented Jan 8, 2020

greg0ire commented Jan 8, 2020

alcaeus commented Jan 10, 2020

ostrolucky commented Jan 10, 2020

greg0ire commented Jan 10, 2020

alcaeus commented Jan 10, 2020

Majkl578 Mar 19, 2020 •

edited

Loading

andrew-demb Mar 19, 2020

alcaeus Mar 19, 2020

Majkl578 Mar 19, 2020

greg0ire Mar 19, 2020

Majkl578 Mar 19, 2020

Majkl578 Mar 19, 2020 •

edited

Loading

alcaeus Mar 20, 2020

Majkl578 Mar 20, 2020

alcaeus Mar 23, 2020

	* @param array<array<string,string>> $filtersParameters
	* @param array<string,array<string,string>> $filtersParameters

	private function getMockContainer(string $connectionName, $params = null) : MockObject
	private function getMockContainer(string $connectionName, array $params) : MockObject

Add type declarations where backwards compatible #1122

Add type declarations where backwards compatible #1122

Conversation

greg0ire commented Jan 6, 2020 • edited Loading

ostrolucky left a 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 Jan 7, 2020

alcaeus commented Jan 8, 2020

greg0ire commented Jan 8, 2020

ostrolucky commented Jan 8, 2020

greg0ire commented Jan 8, 2020

alcaeus commented Jan 10, 2020

ostrolucky commented Jan 10, 2020

greg0ire commented Jan 10, 2020

alcaeus commented Jan 10, 2020

Majkl578 Mar 19, 2020 • 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

Majkl578 Mar 19, 2020 • 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

greg0ire commented Jan 6, 2020 •

edited

Loading

Majkl578 Mar 19, 2020 •

edited

Loading

Majkl578 Mar 19, 2020 •

edited

Loading