Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type declarations where backwards compatible #1122

Merged
merged 1 commit into from
Jan 7, 2020
Merged

Add type declarations where backwards compatible #1122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions ConnectionFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,9 @@ public function createConnection(array $params, Configuration $config = null, Ev
* and the platform version is unknown.
* For details have a look at DoctrineBundle issue #673.
*
* @return AbstractPlatform
*
* @throws DBALException
*/
private function getDatabasePlatform(Connection $connection)
private function getDatabasePlatform(Connection $connection) : AbstractPlatform
{
try {
return $connection->getDatabasePlatform();
Expand All @@ -119,7 +117,7 @@ private function getDatabasePlatform(Connection $connection)
/**
* initialize the types
*/
private function initializeTypes()
private function initializeTypes() : void
{
foreach ($this->typesConfig as $typeName => $typeConfig) {
if (Type::hasType($typeName)) {
Expand Down
5 changes: 4 additions & 1 deletion Controller/ProfilerController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,10 @@ public function explainAction($token, $connectionName, $query)
]));
}

private function explainSQLitePlatform(Connection $connection, $query)
/**
* @param mixed[] $query
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look useful

*/
private function explainSQLitePlatform(Connection $connection, array $query)
{
$params = $query['params'];

Expand Down
2 changes: 1 addition & 1 deletion DependencyInjection/Compiler/EntityListenerPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public function process(ContainerBuilder $container)
}
}

private function attachToListener(ContainerBuilder $container, $name, string $class, array $attributes)
private function attachToListener(ContainerBuilder $container, string $name, string $class, array $attributes) : void
{
$listenerId = sprintf('doctrine.orm.%s_listeners.attach_entity_listeners', $name);

Expand Down
2 changes: 1 addition & 1 deletion DependencyInjection/Compiler/ServiceRepositoryCompilerPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ final class ServiceRepositoryCompilerPass implements CompilerPassInterface
{
const REPOSITORY_SERVICE_TAG = 'doctrine.repository_service';

public function process(ContainerBuilder $container)
public function process(ContainerBuilder $container) : void
{
// when ORM is not enabled
if (! $container->hasDefinition('doctrine.orm.container_repository_factory')) {
Expand Down
32 changes: 9 additions & 23 deletions DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public function getConfigTreeBuilder() : TreeBuilder
/**
* Add DBAL section to configuration tree
*/
private function addDbalSection(ArrayNodeDefinition $node)
private function addDbalSection(ArrayNodeDefinition $node) : void
{
$node
->children()
Expand Down Expand Up @@ -103,10 +103,8 @@ private function addDbalSection(ArrayNodeDefinition $node)

/**
* Return the dbal connections node
*
* @return ArrayNodeDefinition
*/
private function getDbalConnectionsNode()
private function getDbalConnectionsNode() : ArrayNodeDefinition
{
$treeBuilder = new TreeBuilder('connections');
$node = $treeBuilder->getRootNode();
Expand Down Expand Up @@ -185,7 +183,7 @@ private function getDbalConnectionsNode()
*
* These keys are available for slave configurations too.
*/
private function configureDbalDriverNode(ArrayNodeDefinition $node)
private function configureDbalDriverNode(ArrayNodeDefinition $node) : void
{
$node
->children()
Expand Down Expand Up @@ -295,7 +293,7 @@ private function configureDbalDriverNode(ArrayNodeDefinition $node)
/**
* Add the ORM section to configuration tree
*/
private function addOrmSection(ArrayNodeDefinition $node)
private function addOrmSection(ArrayNodeDefinition $node) : void
{
$node
->children()
Expand Down Expand Up @@ -377,10 +375,8 @@ private function addOrmSection(ArrayNodeDefinition $node)

/**
* Return ORM target entity resolver node
*
* @return NodeDefinition
*/
private function getOrmTargetEntityResolverNode()
private function getOrmTargetEntityResolverNode() : NodeDefinition
{
$treeBuilder = new TreeBuilder('resolve_target_entities');
$node = $treeBuilder->getRootNode();
Expand All @@ -396,10 +392,8 @@ private function getOrmTargetEntityResolverNode()

/**
* Return ORM entity listener node
*
* @return NodeDefinition
*/
private function getOrmEntityListenersNode()
private function getOrmEntityListenersNode() : NodeDefinition
{
$treeBuilder = new TreeBuilder('entity_listeners');
$node = $treeBuilder->getRootNode();
Expand Down Expand Up @@ -482,10 +476,8 @@ private function getOrmEntityListenersNode()

/**
* Return ORM entity manager node
*
* @return ArrayNodeDefinition
*/
private function getOrmEntityManagersNode()
private function getOrmEntityManagersNode() : ArrayNodeDefinition
{
$treeBuilder = new TreeBuilder('entity_managers');
$node = $treeBuilder->getRootNode();
Expand Down Expand Up @@ -644,12 +636,8 @@ private function getOrmEntityManagersNode()

/**
* Return a ORM cache driver node for an given entity manager
*
* @param string $name
*
* @return ArrayNodeDefinition
*/
private function getOrmCacheDriverNode($name)
private function getOrmCacheDriverNode(string $name) : ArrayNodeDefinition
{
$treeBuilder = new TreeBuilder($name);
$node = $treeBuilder->getRootNode();
Expand All @@ -673,10 +661,8 @@ private function getOrmCacheDriverNode($name)

/**
* Find proxy auto generate modes for their names and int values
*
* @return array
*/
private function getAutoGenerateModes()
private function getAutoGenerateModes() : array
{
$constPrefix = 'AUTOGENERATE_';
$prefixLen = strlen($constPrefix);
Expand Down
4 changes: 1 addition & 3 deletions DependencyInjection/DoctrineExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,10 +760,8 @@ protected function loadOrmCacheDrivers(array $entityManager, ContainerBuilder $c

/**
* Loads a property info extractor for each defined entity manager.
*
* @param string $entityManagerName
*/
private function loadPropertyInfoExtractor($entityManagerName, ContainerBuilder $container)
private function loadPropertyInfoExtractor(string $entityManagerName, ContainerBuilder $container) : void
{
$propertyExtractorDefinition = $container->register(sprintf('doctrine.orm.%s_entity_manager.property_info_extractor', $entityManagerName), DoctrineExtractor::class);
$argumentId = sprintf('doctrine.orm.%s_entity_manager', $entityManagerName);
Expand Down
13 changes: 5 additions & 8 deletions ManagerConfigurator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ class ManagerConfigurator
/** @var string[] */
private $enabledFilters = [];

/** @var string[] */
/** @var array<string,array<string,string>> */
private $filtersParameters = [];

/**
* @param string[] $enabledFilters
* @param string[] $filtersParameters
* @param string[] $enabledFilters
* @param array<string,array<string,string>> $filtersParameters
*/
public function __construct(array $enabledFilters, array $filtersParameters)
{
Expand All @@ -37,7 +37,7 @@ public function configure(EntityManagerInterface $entityManager)
/**
* Enables filters for a given entity manager
*/
private function enableFilters(EntityManagerInterface $entityManager)
private function enableFilters(EntityManagerInterface $entityManager) : void
{
if (empty($this->enabledFilters)) {
return;
Expand All @@ -56,11 +56,8 @@ private function enableFilters(EntityManagerInterface $entityManager)

/**
* Sets default parameters for a given filter
*
* @param string $name Filter name
* @param SQLFilter $filter Filter object
*/
private function setFilterParameters($name, SQLFilter $filter)
private function setFilterParameters(string $name, SQLFilter $filter) : void
{
if (empty($this->filtersParameters[$name])) {
return;
Expand Down
24 changes: 4 additions & 20 deletions Mapping/DisconnectedMetadataFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 +126,9 @@ public function findNamespaceAndPathForMetadata(ClassMetadataCollection $metadat
/**
* Get a base path for a class
*
* @param string $name class name
* @param string $namespace class namespace
* @param string $path class path
*
* @return string
*
* @throws RuntimeException When base path not found.
*/
private function getBasePathForClass($name, $namespace, $path)
private function getBasePathForClass(string $name, string $namespace, string $path) : string
{
$namespace = str_replace('\\', '/', $namespace);
$search = str_replace('\\', '/', $path);
Expand All @@ -147,12 +141,7 @@ private function getBasePathForClass($name, $namespace, $path)
return $destination;
}

/**
* @param string $namespace
*
* @return ClassMetadataCollection
*/
private function getMetadataForNamespace($namespace)
private function getMetadataForNamespace(string $namespace) : ClassMetadataCollection
{
$metadata = [];
foreach ($this->getAllMetadata() as $m) {
Expand All @@ -166,12 +155,7 @@ private function getMetadataForNamespace($namespace)
return new ClassMetadataCollection($metadata);
}

/**
* @param string $entity
*
* @return ClassMetadataCollection
*/
private function getMetadataForClass($entity)
private function getMetadataForClass(string $entity) : ClassMetadataCollection
{
foreach ($this->registry->getManagers() as $em) {
$cmf = new DisconnectedClassMetadataFactory();
Expand All @@ -188,7 +172,7 @@ private function getMetadataForClass($entity)
/**
* @return ClassMetadata[]
*/
private function getAllMetadata()
private function getAllMetadata() : array
{
$metadata = [];
foreach ($this->registry->getManagers() as $em) {
Expand Down
8 changes: 5 additions & 3 deletions Repository/ContainerRepositoryFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public function __construct(ContainerInterface $container = null)
/**
* {@inheritdoc}
*/
public function getRepository(EntityManagerInterface $entityManager, $entityName)
public function getRepository(EntityManagerInterface $entityManager, $entityName) : ObjectRepository
Copy link
Contributor

@Majkl578 Majkl578 Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Precisely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@Majkl578 Majkl578 Mar 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requirement for the ObjectRepository/EntityRepository was never documented:

It was also never validated:

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

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

{
$metadata = $entityManager->getClassMetadata($entityName);
$repositoryServiceId = $metadata->customRepositoryClassName;
Expand Down Expand Up @@ -72,8 +72,10 @@ public function getRepository(EntityManagerInterface $entityManager, $entityName
return $this->getOrCreateRepository($entityManager, $metadata);
}

private function getOrCreateRepository(EntityManagerInterface $entityManager, ClassMetadata $metadata)
{
private function getOrCreateRepository(
EntityManagerInterface $entityManager,
ClassMetadata $metadata
) : ObjectRepository {
$repositoryHash = $metadata->getName() . spl_object_hash($entityManager);
if (isset($this->managedRepositories[$repositoryHash])) {
return $this->managedRepositories[$repositoryHash];
Expand Down
14 changes: 7 additions & 7 deletions Tests/Builder/BundleConfigurationBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public static function createBuilderWithBaseValues()
return $builder;
}

public function addBaseConnection()
public function addBaseConnection() : self
{
$this->addConnection([
'connections' => [
Expand All @@ -32,7 +32,7 @@ public function addBaseConnection()
return $this;
}

public function addBaseEntityManager()
public function addBaseEntityManager() : self
{
$this->addEntityManager([
'default_entity_manager' => 'default',
Expand All @@ -48,7 +48,7 @@ public function addBaseEntityManager()
return $this;
}

public function addBaseSecondLevelCache()
public function addBaseSecondLevelCache() : self
{
$this->addSecondLevelCache([
'region_cache_driver' => ['type' => 'pool', 'pool' => 'my_pool'],
Expand All @@ -60,28 +60,28 @@ public function addBaseSecondLevelCache()
return $this;
}

public function addConnection($config)
public function addConnection($config) : self
{
$this->configuration['dbal'] = $config;

return $this;
}

public function addEntityManager($config)
public function addEntityManager($config) : self
{
$this->configuration['orm'] = $config;

return $this;
}

public function addSecondLevelCache($config, $manager = 'default')
public function addSecondLevelCache($config, $manager = 'default') : self
{
$this->configuration['orm']['entity_managers'][$manager]['second_level_cache'] = $config;

return $this;
}

public function build()
public function build() : array
{
return $this->configuration;
}
Expand Down
9 changes: 3 additions & 6 deletions Tests/Command/CreateDatabaseDoctrineTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
namespace Doctrine\Bundle\DoctrineBundle\Tests\Command;

use Doctrine\Bundle\DoctrineBundle\Command\CreateDatabaseDoctrineCommand;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use PHPUnit_Framework_MockObject_MockObject;
use Symfony\Component\Console\Application;
use Symfony\Component\Console\Tester\CommandTester;

Expand Down Expand Up @@ -86,12 +86,9 @@ public function testExecuteWithShardOption() : void
}

/**
* @param string $connectionName Connection name
* @param mixed[]|null $params Connection parameters
*
* @return PHPUnit_Framework_MockObject_MockObject
* @param mixed[]|null $params Connection parameters
*/
private function getMockContainer($connectionName, $params = null)
private function getMockContainer(string $connectionName, array $params = null) : MockObject
{
// Mock the container and everything you'll need here
$mockDoctrine = $this->getMockBuilder('Doctrine\Persistence\ManagerRegistry')
Expand Down
9 changes: 3 additions & 6 deletions Tests/Command/DropDatabaseDoctrineTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
namespace Doctrine\Bundle\DoctrineBundle\Tests\Command;

use Doctrine\Bundle\DoctrineBundle\Command\DropDatabaseDoctrineCommand;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use PHPUnit_Framework_MockObject_MockObject;
use Symfony\Component\Console\Application;
use Symfony\Component\Console\Tester\CommandTester;

Expand Down Expand Up @@ -75,12 +75,9 @@ public function testExecuteWithoutOptionForceWillFailWithAttentionMessage() : vo
}

/**
* @param string $connectionName Connection name
* @param array|null $params Connection parameters
*
* @return PHPUnit_Framework_MockObject_MockObject
* @param array|null $params Connection parameters
*/
private function getMockContainer($connectionName, $params = null)
private function getMockContainer(string $connectionName, array $params = null) : MockObject
{
// Mock the container and everything you'll need here
$mockDoctrine = $this->getMockBuilder('Doctrine\Persistence\ManagerRegistry')
Expand Down
Loading