From 5e6c812c449e18df4a69852d441e7308a713c3cf Mon Sep 17 00:00:00 2001 From: mscherer Date: Wed, 14 Mar 2018 11:16:45 +0100 Subject: [PATCH] Deprecate getter code smell parts contain() in favor of getContain(). --- src/ORM/Association.php | 6 ++-- src/ORM/EagerLoader.php | 26 ++++++++++++-- src/ORM/LazyEagerLoader.php | 2 +- src/ORM/Query.php | 25 ++++++++++--- .../TestCase/ORM/Association/HasManyTest.php | 6 +++- tests/TestCase/ORM/EagerLoaderTest.php | 36 +++++++++---------- tests/TestCase/ORM/QueryTest.php | 19 +++++----- 7 files changed, 81 insertions(+), 39 deletions(-) diff --git a/src/ORM/Association.php b/src/ORM/Association.php index 9c03c2b9e82..dd94f287568 100644 --- a/src/ORM/Association.php +++ b/src/ORM/Association.php @@ -1262,7 +1262,7 @@ protected function _formatAssociationResults($query, $surrogate, $options) protected function _bindNewAssociations($query, $surrogate, $options) { $loader = $surrogate->getEagerLoader(); - $contain = $loader->contain(); + $contain = $loader->getContain(); $matching = $loader->getMatching(); if (!$contain && !$matching) { @@ -1275,7 +1275,9 @@ protected function _bindNewAssociations($query, $surrogate, $options) } $eagerLoader = $query->getEagerLoader(); - $eagerLoader->contain($newContain); + if ($newContain) { + $eagerLoader->contain($newContain); + } foreach ($matching as $alias => $value) { $eagerLoader->setMatching( diff --git a/src/ORM/EagerLoader.php b/src/ORM/EagerLoader.php index ca6546e3f91..449b9b02e4b 100644 --- a/src/ORM/EagerLoader.php +++ b/src/ORM/EagerLoader.php @@ -113,6 +113,8 @@ class EagerLoader * this allows this object to calculate joins or any additional queries that * must be executed to bring the required associated data. * + * The getter part is deprecated as of 3.6.0. Use getContain() instead. + * * Accepted options per passed association: * * - foreignKey: Used to set a different field to match both tables, if set to false @@ -134,7 +136,12 @@ class EagerLoader public function contain($associations = [], callable $queryBuilder = null) { if (empty($associations)) { - return $this->_containments; + deprecationWarning( + 'Using EagerLoader::contain() as getter is deprecated. ' . + 'Use getContain() instead.' + ); + + return $this->getContain(); } if ($queryBuilder) { @@ -160,6 +167,19 @@ public function contain($associations = [], callable $queryBuilder = null) return $this->_containments = $associations; } + /** + * Gets the list of associations that should be eagerly loaded along for a + * specific table using when a query is provided. The list of associated tables + * passed to this method must have been previously set as associations using the + * Table API. + * + * @return array Containments. + */ + public function getContain() + { + return $this->_containments; + } + /** * Remove any existing non-matching based containments. * @@ -276,7 +296,7 @@ public function getMatching() $this->_matching = new static(); } - return $this->_matching->contain(); + return $this->_matching->getContain(); } /** @@ -705,7 +725,7 @@ public function associationsMap($table) { $map = []; - if (!$this->getMatching() && !$this->contain() && empty($this->_joinsMap)) { + if (!$this->getMatching() && !$this->getContain() && empty($this->_joinsMap)) { return $map; } diff --git a/src/ORM/LazyEagerLoader.php b/src/ORM/LazyEagerLoader.php index 3fc743b0a4e..d22965b4b46 100644 --- a/src/ORM/LazyEagerLoader.php +++ b/src/ORM/LazyEagerLoader.php @@ -51,7 +51,7 @@ public function loadInto($entities, array $contain, Table $source) $entities = new Collection($entities); $query = $this->_getQuery($entities, $contain, $source); - $associations = array_keys($query->contain()); + $associations = array_keys($query->getContain()); $entities = $this->_injectResults($entities, $query, $associations, $source); diff --git a/src/ORM/Query.php b/src/ORM/Query.php index f61295adfbc..5a08032385f 100644 --- a/src/ORM/Query.php +++ b/src/ORM/Query.php @@ -408,7 +408,7 @@ public function eagerLoader(EagerLoader $instance = null) * * If called with no arguments, this function will return an array with * with the list of previously configured associations to be contained in the - * result. + * result. This getter part is deprecated as of 3.6.0. Use getContain() instead. * * If called with an empty first argument and `$override` is set to true, the * previous list will be emptied. @@ -428,7 +428,12 @@ public function contain($associations = null, $override = false) } if ($associations === null) { - return $loader->contain(); + deprecationWarning( + 'Using Query::contain() as getter is deprecated. ' . + 'Use getContain() instead.' + ); + + return $loader->getContain(); } $queryBuilder = null; @@ -436,12 +441,22 @@ public function contain($associations = null, $override = false) $queryBuilder = $override; } - $result = $loader->contain($associations, $queryBuilder); - $this->_addAssociationsToTypeMap($this->repository(), $this->getTypeMap(), $result); + if ($associations) { + $loader->contain($associations, $queryBuilder); + } + $this->_addAssociationsToTypeMap($this->repository(), $this->getTypeMap(), $loader->getContain()); return $this; } + /** + * @return array + */ + public function getContain() + { + return $this->getEagerLoader()->getContain(); + } + /** * Clears the contained associations from the current query. * @@ -1258,7 +1273,7 @@ public function __debugInfo() 'buffered' => $this->_useBufferedResults, 'formatters' => count($this->_formatters), 'mapReducers' => count($this->_mapReduce), - 'contain' => $eagerLoader ? $eagerLoader->contain() : [], + 'contain' => $eagerLoader ? $eagerLoader->getContain() : [], 'matching' => $eagerLoader ? $eagerLoader->getMatching() : [], 'extraOptions' => $this->_options, 'repository' => $this->_repository diff --git a/tests/TestCase/ORM/Association/HasManyTest.php b/tests/TestCase/ORM/Association/HasManyTest.php index 1b8847e2c5c..3c4619438dd 100644 --- a/tests/TestCase/ORM/Association/HasManyTest.php +++ b/tests/TestCase/ORM/Association/HasManyTest.php @@ -303,6 +303,8 @@ public function testEagerLoaderWithOverrides() $association = new HasMany('Articles', $config); $keys = [1, 2, 3, 4]; + + /** @var \Cake\ORM\Query $query */ $query = $this->article->query(); $query->addDefaultTypes($this->article->Comments->getSource()); @@ -336,7 +338,7 @@ public function testEagerLoaderWithOverrides() $expected = new OrderByExpression(['title' => 'DESC']); $this->assertOrderClause($expected, $query); - $this->assertArrayHasKey('Comments', $query->contain()); + $this->assertArrayHasKey('Comments', $query->getContain()); } /** @@ -382,6 +384,8 @@ public function testEagerLoaderWithQueryBuilder() ]; $association = new HasMany('Articles', $config); $keys = [1, 2, 3, 4]; + + /** @var \Cake\ORM\Query $query */ $query = $this->article->query(); $this->article->method('find') ->with('all') diff --git a/tests/TestCase/ORM/EagerLoaderTest.php b/tests/TestCase/ORM/EagerLoaderTest.php index 5621897eb35..985d74c1fda 100644 --- a/tests/TestCase/ORM/EagerLoaderTest.php +++ b/tests/TestCase/ORM/EagerLoaderTest.php @@ -235,7 +235,7 @@ public function testContainToJoinsOneLevel() ]]) ->will($this->returnValue($query)); - $loader = new EagerLoader; + $loader = new EagerLoader(); $loader->contain($contains); $query->select('foo.id')->setEagerLoader($loader)->sql(); } @@ -248,7 +248,7 @@ public function testContainToJoinsOneLevel() */ public function testContainDotNotation() { - $loader = new EagerLoader; + $loader = new EagerLoader(); $loader->contain([ 'clients.orders.stuff', 'clients.companies.categories' => ['conditions' => ['a >' => 1]] @@ -265,7 +265,7 @@ public function testContainDotNotation() ] ] ]; - $this->assertEquals($expected, $loader->contain()); + $this->assertEquals($expected, $loader->getContain()); $loader->contain([ 'clients.orders' => ['fields' => ['a', 'b']], 'clients' => ['sort' => ['a' => 'desc']], @@ -273,7 +273,7 @@ public function testContainDotNotation() $expected['clients']['orders'] += ['fields' => ['a', 'b']]; $expected['clients'] += ['sort' => ['a' => 'desc']]; - $this->assertEquals($expected, $loader->contain()); + $this->assertEquals($expected, $loader->getContain()); } /** @@ -283,7 +283,7 @@ public function testContainDotNotation() */ public function testContainKeyValueNotation() { - $loader = new EagerLoader; + $loader = new EagerLoader(); $loader->contain([ 'clients', 'companies' => 'categories', @@ -296,7 +296,7 @@ public function testContainKeyValueNotation() ], ], ]; - $this->assertEquals($expected, $loader->contain()); + $this->assertEquals($expected, $loader->getContain()); } /** @@ -308,7 +308,7 @@ public function testContainClosure() { $builder = function ($query) { }; - $loader = new EagerLoader; + $loader = new EagerLoader(); $loader->contain([ 'clients.orders.stuff' => ['fields' => ['a']], 'clients' => $builder @@ -322,14 +322,14 @@ public function testContainClosure() 'queryBuilder' => $builder ] ]; - $this->assertEquals($expected, $loader->contain()); + $this->assertEquals($expected, $loader->getContain()); - $loader = new EagerLoader; + $loader = new EagerLoader(); $loader->contain([ 'clients.orders.stuff' => ['fields' => ['a']], 'clients' => ['queryBuilder' => $builder] ]); - $this->assertEquals($expected, $loader->contain()); + $this->assertEquals($expected, $loader->getContain()); } /** @@ -341,7 +341,7 @@ public function testContainSecondSignature() { $builder = function ($query) { }; - $loader = new EagerLoader; + $loader = new EagerLoader(); $loader->contain('clients', $builder); $expected = [ @@ -349,7 +349,7 @@ public function testContainSecondSignature() 'queryBuilder' => $builder ] ]; - $this->assertEquals($expected, $loader->contain()); + $this->assertEquals($expected, $loader->getContain()); } /** @@ -363,7 +363,7 @@ public function testContainSecondSignatureInvalid() $builder = function ($query) { }; - $loader = new EagerLoader; + $loader = new EagerLoader(); $loader->contain(['clients'], $builder); $expected = [ @@ -371,7 +371,7 @@ public function testContainSecondSignatureInvalid() 'queryBuilder' => $builder ] ]; - $this->assertEquals($expected, $loader->contain()); + $this->assertEquals($expected, $loader->getContain()); } /** @@ -381,7 +381,7 @@ public function testContainSecondSignatureInvalid() */ public function testContainMergeBuilders() { - $loader = new EagerLoader; + $loader = new EagerLoader(); $loader->contain([ 'clients' => function ($query) { return $query->select(['a']); @@ -392,7 +392,7 @@ public function testContainMergeBuilders() return $query->select(['b']); } ]); - $builder = $loader->contain()['clients']['queryBuilder']; + $builder = $loader->getContain()['clients']['queryBuilder']; $table = $this->getTableLocator()->get('foo'); $query = new Query($this->connection, $table); $query = $builder($query); @@ -417,7 +417,7 @@ public function testContainToFieldsPredefined() $table = $this->getTableLocator()->get('foo'); $query = new Query($this->connection, $table); - $loader = new EagerLoader; + $loader = new EagerLoader(); $loader->contain($contains); $query->select('foo.id'); $loader->attachAssociations($query, $table, true); @@ -502,7 +502,7 @@ public function testNormalizedPath() ->setConstructorArgs([$this->connection, $this->table]) ->getMock(); - $loader = new EagerLoader; + $loader = new EagerLoader(); $loader->contain($contains); $normalized = $loader->normalized($this->table); $this->assertEquals('clients', $normalized['clients']->aliasPath()); diff --git a/tests/TestCase/ORM/QueryTest.php b/tests/TestCase/ORM/QueryTest.php index 21d7cf23502..3f615ab8159 100644 --- a/tests/TestCase/ORM/QueryTest.php +++ b/tests/TestCase/ORM/QueryTest.php @@ -923,7 +923,7 @@ public function testApplyOptions() $this->assertEquals($expected, $query->clause('having')); $expected = ['articles' => []]; - $this->assertEquals($expected, $query->contain()); + $this->assertEquals($expected, $query->getContain()); } /** @@ -1815,18 +1815,19 @@ public function testClearContain() ->setConstructorArgs([$this->connection, $this->table]) ->getMock(); + /** @var \Cake\ORM\Query $query */ $query->contain([ 'Articles' ]); - $result = $query->contain(); + $result = $query->getContain(); $this->assertInternalType('array', $result); $this->assertNotEmpty($result); $result = $query->clearContain(); $this->assertInstanceOf(Query::class, $result); - $result = $query->contain(); + $result = $query->getContain(); $this->assertInternalType('array', $result); $this->assertEmpty($result); } @@ -1983,13 +1984,13 @@ public function testContainOverwrite() $query = $table->find(); $query->contain(['Comments']); - $this->assertEquals(['Comments'], array_keys($query->contain())); + $this->assertEquals(['Comments'], array_keys($query->getContain())); $query->contain(['Authors'], true); - $this->assertEquals(['Authors'], array_keys($query->contain())); + $this->assertEquals(['Authors'], array_keys($query->getContain())); $query->contain(['Comments', 'Authors'], true); - $this->assertEquals(['Comments', 'Authors'], array_keys($query->contain())); + $this->assertEquals(['Comments', 'Authors'], array_keys($query->getContain())); } /** @@ -3540,8 +3541,8 @@ public function testNotMatchingNested() $results = $table->find() ->enableHydration(false) - ->matching('articles', function ($q) { - return $q->notMatching('tags', function ($q) { + ->matching('articles', function (Query $q) { + return $q->notMatching('tags', function (Query $q) { return $q->where(['tags.name' => 'tag3']); }); }) @@ -3599,7 +3600,7 @@ public function testSelectAllExceptWithContains() $result = $table ->find() ->contain([ - 'Comments' => function ($query) use ($table) { + 'Comments' => function (Query $query) use ($table) { return $query->selectAllExcept($table->Comments, ['published']); } ])