Completed
Pull Request — master (#905)
by Gabriel
01:56
created

DoctrineDataCollector   B

Complexity

Total Complexity 43

Size/Duplication

Total Lines 247
Duplicated Lines 8.5 %

Coupling/Cohesion

Components 1
Dependencies 8

Importance

Changes 0
Metric Value
wmc 43
lcom 1
cbo 8
dl 21
loc 247
rs 8.96
c 0
b 0
f 0

14 Methods

Rating   Name   Duplication   Size   Complexity  
A __construct() 0 6 1
F collect() 21 116 18
A getEntities() 0 4 1
A getMappingErrors() 0 4 1
A getCacheHitsCount() 0 4 1
A getCachePutsCount() 0 4 1
A getCacheMissesCount() 0 4 1
A getCacheEnabled() 0 4 1
A getCacheRegions() 0 4 1
A getCacheCounts() 0 4 1
A getInvalidEntityCount() 0 8 2
B getGroupedQueries() 0 40 9
A executionTimePercentage() 0 8 3
A getGroupedQueryCount() 0 9 2

How to fix   Duplicated Code    Complexity   

Duplicated Code

Duplicate code is one of the most pungent code smells. A rule that is often used is to re-structure code once it is duplicated in three or more places.

Common duplication problems, and corresponding solutions are:

Complex Class

 Tip:   Before tackling complexity, make sure that you eliminate any duplication first. This often can reduce the size of classes significantly.

Complex classes like DoctrineDataCollector often do a lot of different things. To break such a class down, we need to identify a cohesive component within that class. A common approach to find such a component is to look for fields/methods that share the same prefixes, or suffixes. You can also have a look at the cohesion graph to spot any un-connected, or weakly-connected components.

Once you have determined the fields that belong together, you can apply the Extract Class refactoring. If the component makes sense as a sub-class, Extract Subclass is also a candidate, and is often faster.

While breaking up the class, it is a good idea to analyze how other classes use DoctrineDataCollector, and based on these observations, apply Extract Interface, too.

1
<?php
2
3
namespace Doctrine\Bundle\DoctrineBundle\DataCollector;
4
5
use Doctrine\Common\Persistence\ManagerRegistry;
6
use Doctrine\ORM\Cache\Logging\CacheLoggerChain;
7
use Doctrine\ORM\Cache\Logging\StatisticsCacheLogger;
8
use Doctrine\ORM\Configuration;
9
use Doctrine\ORM\EntityManager;
10
use Doctrine\ORM\Mapping\ClassMetadataFactory;
11
use Doctrine\ORM\Mapping\ClassMetadataInfo;
12
use Doctrine\ORM\Tools\SchemaValidator;
13
use Doctrine\ORM\Version;
14
use Exception;
15
use Symfony\Bridge\Doctrine\DataCollector\DoctrineDataCollector as BaseCollector;
16
use Symfony\Component\HttpFoundation\Request;
17
use Symfony\Component\HttpFoundation\Response;
18
19
/**
20
 * DoctrineDataCollector.
21
 */
22
class DoctrineDataCollector extends BaseCollector
23
{
24
    /** @var ManagerRegistry */
25
    private $registry;
0 ignored issues
show
Comprehensibility introduced by
Consider using a different property name as you override a private property of the parent class.
Loading history...
26
27
    /** @var int|null */
28
    private $invalidEntityCount;
29
30
    /** @var string[] */
31
    private $groupedQueries;
32
33
    public function __construct(ManagerRegistry $registry)
34
    {
35
        $this->registry = $registry;
36
37
        parent::__construct($registry);
38
    }
39
40
    /**
41
     * {@inheritdoc}
42
     */
43
    public function collect(Request $request, Response $response, Exception $exception = null)
44
    {
45
        parent::collect($request, $response, $exception);
46
47
        $errors   = [];
48
        $entities = [];
49
        $caches   = [
50
            'enabled' => false,
51
            'log_enabled' => false,
52
            'counts' => [
53
                'puts' => 0,
54
                'hits' => 0,
55
                'misses' => 0,
56
            ],
57
            'regions' => [
58
                'puts' => [],
59
                'hits' => [],
60
                'misses' => [],
61
            ],
62
        ];
63
64
        /** @var EntityManager $em */
65
        foreach ($this->registry->getManagers() as $name => $em) {
66
            $entities[$name] = [];
67
            /** @var ClassMetadataFactory $factory */
68
            $factory   = $em->getMetadataFactory();
69
            $validator = new SchemaValidator($em);
0 ignored issues
show
Compatibility introduced by
$em of type object<Doctrine\Common\Persistence\ObjectManager> is not a sub-type of object<Doctrine\ORM\EntityManagerInterface>. It seems like you assume a child interface of the interface Doctrine\Common\Persistence\ObjectManager to be always present.

This check looks for parameters that are defined as one type in their type hint or doc comment but seem to be used as a narrower type, i.e an implementation of an interface or a subclass.

Consider changing the type of the parameter or doing an instanceof check before assuming your parameter is of the expected type.

Loading history...
70
71
            /** @var ClassMetadataInfo $class */
72
            foreach ($factory->getLoadedMetadata() as $class) {
73
                if (isset($entities[$name][$class->getName()])) {
74
                    continue;
75
                }
76
77
                $classErrors                        = $validator->validateClass($class);
0 ignored issues
show
Compatibility introduced by
$class of type object<Doctrine\Common\P...\Mapping\ClassMetadata> is not a sub-type of object<Doctrine\ORM\Mapping\ClassMetadataInfo>. It seems like you assume a concrete implementation of the interface Doctrine\Common\Persistence\Mapping\ClassMetadata to be always present.

This check looks for parameters that are defined as one type in their type hint or doc comment but seem to be used as a narrower type, i.e an implementation of an interface or a subclass.

Consider changing the type of the parameter or doing an instanceof check before assuming your parameter is of the expected type.

Loading history...
78
                $entities[$name][$class->getName()] = $class->getName();
79
80
                if (empty($classErrors)) {
81
                    continue;
82
                }
83
84
                $errors[$name][$class->getName()] = $classErrors;
85
            }
86
87
            if (version_compare(Version::VERSION, '2.5.0-DEV') < 0) {
88
                continue;
89
            }
90
91
            /** @var Configuration $emConfig */
92
            $emConfig   = $em->getConfiguration();
0 ignored issues
show
Bug introduced by
It seems like you code against a concrete implementation and not the interface Doctrine\Common\Persistence\ObjectManager as the method getConfiguration() does only exist in the following implementations of said interface: Doctrine\ORM\Decorator\EntityManagerDecorator, Doctrine\ORM\EntityManager.

Let’s take a look at an example:

interface User
{
    /** @return string */
    public function getPassword();
}

class MyUser implements User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different implementation of User which does not have a getDisplayName() method, the code will break.

Available Fixes

  1. Change the type-hint for the parameter:

    class AuthSystem
    {
        public function authenticate(MyUser $user) { /* ... */ }
    }
    
  2. Add an additional type-check:

    class AuthSystem
    {
        public function authenticate(User $user)
        {
            if ($user instanceof MyUser) {
                $this->logger->info(/** ... */);
            }
    
            // or alternatively
            if ( ! $user instanceof MyUser) {
                throw new \LogicException(
                    '$user must be an instance of MyUser, '
                   .'other instances are not supported.'
                );
            }
    
        }
    }
    
Note: PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case.
  1. Add the method to the interface:

    interface User
    {
        /** @return string */
        public function getPassword();
    
        /** @return string */
        public function getDisplayName();
    }
    
Loading history...
93
            $slcEnabled = $emConfig->isSecondLevelCacheEnabled();
94
95
            if (! $slcEnabled) {
96
                continue;
97
            }
98
99
            $caches['enabled'] = true;
100
101
            /** @var $cacheConfiguration \Doctrine\ORM\Cache\CacheConfiguration */
102
            /** @var CacheLoggerChain $cacheLoggerChain */
103
            $cacheConfiguration = $emConfig->getSecondLevelCacheConfiguration();
104
            $cacheLoggerChain   = $cacheConfiguration->getCacheLogger();
105
106
            if (! $cacheLoggerChain || ! $cacheLoggerChain->getLogger('statistics')) {
107
                continue;
108
            }
109
110
            /** @var StatisticsCacheLogger $cacheLoggerStats */
111
            $cacheLoggerStats      = $cacheLoggerChain->getLogger('statistics');
112
            $caches['log_enabled'] = true;
113
114
            $caches['counts']['puts']   += $cacheLoggerStats->getPutCount();
115
            $caches['counts']['hits']   += $cacheLoggerStats->getHitCount();
116
            $caches['counts']['misses'] += $cacheLoggerStats->getMissCount();
117
118 View Code Duplication
            foreach ($cacheLoggerStats->getRegionsPut() as $key => $value) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
119
                if (! isset($caches['regions']['puts'][$key])) {
120
                    $caches['regions']['puts'][$key] = 0;
121
                }
122
123
                $caches['regions']['puts'][$key] += $value;
124
            }
125
126 View Code Duplication
            foreach ($cacheLoggerStats->getRegionsHit() as $key => $value) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
127
                if (! isset($caches['regions']['hits'][$key])) {
128
                    $caches['regions']['hits'][$key] = 0;
129
                }
130
131
                $caches['regions']['hits'][$key] += $value;
132
            }
133
134 View Code Duplication
            foreach ($cacheLoggerStats->getRegionsMiss() as $key => $value) {
0 ignored issues
show
Duplication introduced by
This code seems to be duplicated across your project.

Duplicated code is one of the most pungent code smells. If you need to duplicate the same code in three or more different places, we strongly encourage you to look into extracting the code into a single class or operation.

You can also find more detailed suggestions in the “Code” section of your repository.

Loading history...
135
                if (! isset($caches['regions']['misses'][$key])) {
136
                    $caches['regions']['misses'][$key] = 0;
137
                }
138
139
                $caches['regions']['misses'][$key] += $value;
140
            }
141
        }
142
143
        // HttpKernel < 3.2 compatibility layer
144
        if (method_exists($this, 'cloneVar')) {
145
            // Might be good idea to replicate this block in doctrine bridge so we can drop this from here after some time.
146
            // This code is compatible with such change, because cloneVar is supposed to check if input is already cloned.
147
            foreach ($this->data['queries'] as &$queries) {
148
                foreach ($queries as &$query) {
149
                    $query['params'] = $this->cloneVar($query['params']);
150
                }
151
            }
152
        }
153
154
        $this->data['entities'] = $entities;
155
        $this->data['errors']   = $errors;
156
        $this->data['caches']   = $caches;
157
        $this->groupedQueries   = null;
0 ignored issues
show
Documentation Bug introduced by
It seems like null of type null is incompatible with the declared type array<integer,string> of property $groupedQueries.

Our type inference engine has found an assignment to a property that is incompatible with the declared type of that property.

Either this assignment is in error or the assigned type should be added to the documentation/type hint for that property..

Loading history...
158
    }
159
160
    public function getEntities()
0 ignored issues
show
Documentation introduced by
The return type could not be reliably inferred; please add a @return annotation.

Our type inference engine in quite powerful, but sometimes the code does not provide enough clues to go by. In these cases we request you to add a @return annotation as described here.

Loading history...
161
    {
162
        return $this->data['entities'];
163
    }
164
165
    public function getMappingErrors()
0 ignored issues
show
Documentation introduced by
The return type could not be reliably inferred; please add a @return annotation.

Our type inference engine in quite powerful, but sometimes the code does not provide enough clues to go by. In these cases we request you to add a @return annotation as described here.

Loading history...
166
    {
167
        return $this->data['errors'];
168
    }
169
170
    public function getCacheHitsCount()
0 ignored issues
show
Documentation introduced by
The return type could not be reliably inferred; please add a @return annotation.

Our type inference engine in quite powerful, but sometimes the code does not provide enough clues to go by. In these cases we request you to add a @return annotation as described here.

Loading history...
171
    {
172
        return $this->data['caches']['counts']['hits'];
173
    }
174
175
    public function getCachePutsCount()
0 ignored issues
show
Documentation introduced by
The return type could not be reliably inferred; please add a @return annotation.

Our type inference engine in quite powerful, but sometimes the code does not provide enough clues to go by. In these cases we request you to add a @return annotation as described here.

Loading history...
176
    {
177
        return $this->data['caches']['counts']['puts'];
178
    }
179
180
    public function getCacheMissesCount()
0 ignored issues
show
Documentation introduced by
The return type could not be reliably inferred; please add a @return annotation.

Our type inference engine in quite powerful, but sometimes the code does not provide enough clues to go by. In these cases we request you to add a @return annotation as described here.

Loading history...
181
    {
182
        return $this->data['caches']['counts']['misses'];
183
    }
184
185
    public function getCacheEnabled()
0 ignored issues
show
Documentation introduced by
The return type could not be reliably inferred; please add a @return annotation.

Our type inference engine in quite powerful, but sometimes the code does not provide enough clues to go by. In these cases we request you to add a @return annotation as described here.

Loading history...
186
    {
187
        return $this->data['caches']['enabled'];
188
    }
189
190
    public function getCacheRegions()
0 ignored issues
show
Documentation introduced by
The return type could not be reliably inferred; please add a @return annotation.

Our type inference engine in quite powerful, but sometimes the code does not provide enough clues to go by. In these cases we request you to add a @return annotation as described here.

Loading history...
191
    {
192
        return $this->data['caches']['regions'];
193
    }
194
195
    public function getCacheCounts()
0 ignored issues
show
Documentation introduced by
The return type could not be reliably inferred; please add a @return annotation.

Our type inference engine in quite powerful, but sometimes the code does not provide enough clues to go by. In these cases we request you to add a @return annotation as described here.

Loading history...
196
    {
197
        return $this->data['caches']['counts'];
198
    }
199
200
    public function getInvalidEntityCount()
201
    {
202
        if ($this->invalidEntityCount === null) {
203
            $this->invalidEntityCount = array_sum(array_map('count', $this->data['errors']));
0 ignored issues
show
Documentation Bug introduced by
It seems like array_sum(array_map('cou...$this->data['errors'])) can also be of type double. However, the property $invalidEntityCount is declared as type integer|null. Maybe add an additional type check?

Our type inference engine has found a suspicous assignment of a value to a property. This check raises an issue when a value that can be of a mixed type is assigned to a property that is type hinted more strictly.

For example, imagine you have a variable $accountId that can either hold an Id object or false (if there is no account id yet). Your code now assigns that value to the id property of an instance of the Account class. This class holds a proper account, so the id value must no longer be false.

Either this assignment is in error or a type check should be added for that assignment.

class Id
{
    public $id;

    public function __construct($id)
    {
        $this->id = $id;
    }

}

class Account
{
    /** @var  Id $id */
    public $id;
}

$account_id = false;

if (starsAreRight()) {
    $account_id = new Id(42);
}

$account = new Account();
if ($account instanceof Id)
{
    $account->id = $account_id;
}
Loading history...
204
        }
205
206
        return $this->invalidEntityCount;
207
    }
208
209
    public function getGroupedQueries()
210
    {
211
        if ($this->groupedQueries !== null) {
212
            return $this->groupedQueries;
213
        }
214
215
        $this->groupedQueries = [];
216
        $totalExecutionMS     = 0;
217
        foreach ($this->data['queries'] as $connection => $queries) {
218
            $connectionGroupedQueries = [];
219
            foreach ($queries as $i => $query) {
220
                $key = $query['sql'];
221
                if (! isset($connectionGroupedQueries[$key])) {
222
                    $connectionGroupedQueries[$key]                = $query;
223
                    $connectionGroupedQueries[$key]['executionMS'] = 0;
224
                    $connectionGroupedQueries[$key]['count']       = 0;
225
                    $connectionGroupedQueries[$key]['index']       = $i; // "Explain query" relies on query index in 'queries'.
226
                }
227
                $connectionGroupedQueries[$key]['executionMS'] += $query['executionMS'];
228
                $connectionGroupedQueries[$key]['count']++;
229
                $totalExecutionMS += $query['executionMS'];
230
            }
231
            usort($connectionGroupedQueries, static function ($a, $b) {
232
                if ($a['executionMS'] === $b['executionMS']) {
233
                    return 0;
234
                }
235
                return $a['executionMS'] < $b['executionMS'] ? 1 : -1;
236
            });
237
            $this->groupedQueries[$connection] = $connectionGroupedQueries;
238
        }
239
240
        foreach ($this->groupedQueries as $connection => $queries) {
241
            foreach ($queries as $i => $query) {
242
                $this->groupedQueries[$connection][$i]['executionPercent'] =
243
                    $this->executionTimePercentage($query['executionMS'], $totalExecutionMS);
244
            }
245
        }
246
247
        return $this->groupedQueries;
248
    }
249
250
    private function executionTimePercentage($executionTimeMS, $totalExecutionTimeMS)
251
    {
252
        if ($totalExecutionTimeMS === 0.0 || $totalExecutionTimeMS === 0) {
253
            return 0;
254
        }
255
256
        return $executionTimeMS / $totalExecutionTimeMS * 100;
257
    }
258
259
    public function getGroupedQueryCount()
260
    {
261
        $count = 0;
262
        foreach ($this->getGroupedQueries() as $connectionGroupedQueries) {
263
            $count += count($connectionGroupedQueries);
264
        }
265
266
        return $count;
267
    }
268
}
269