Deprecate getter code smell parts contain() in favor of getContain().

cakephp · Mar 14, 2018 · 5e6c812 · 5e6c812
1 parent ff1c0d2
commit 5e6c812
Show file tree

Hide file tree

Showing 7 changed files with 81 additions and 39 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,20 +428,35 @@ 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;
         if (is_callable($override)) {
             $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
@@ -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
@@ -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,15 +265,15 @@ public function testContainDotNotation()
                 ]
             ]
         ];
-        $this->assertEquals($expected, $loader->contain());
+        $this->assertEquals($expected, $loader->getContain());
         $loader->contain([
             'clients.orders' => ['fields' => ['a', 'b']],
             'clients' => ['sort' => ['a' => 'desc']],
         ]);
 
         $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,15 +341,15 @@ public function testContainSecondSignature()
     {
         $builder = function ($query) {
         };
-        $loader = new EagerLoader;
+        $loader = new EagerLoader();
         $loader->contain('clients', $builder);
 
         $expected = [
             'clients' => [
                 'queryBuilder' => $builder
             ]
         ];
-        $this->assertEquals($expected, $loader->contain());
+        $this->assertEquals($expected, $loader->getContain());
     }
 
     /**
@@ -363,15 +363,15 @@ public function testContainSecondSignatureInvalid()
 
         $builder = function ($query) {
         };
-        $loader = new EagerLoader;
+        $loader = new EagerLoader();
         $loader->contain(['clients'], $builder);
 
         $expected = [
             'clients' => [
                 '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
@@ -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']);
                 }
             ])