AdminListController   F
last analyzed

Complexity

Total Complexity 60

Size/Duplication

Total Lines 537
Duplicated Lines 32.03 %

Coupling/Cohesion

Components 1
Dependencies 23

Importance

Changes 0
Metric Value
wmc 60
lcom 1
cbo 23
dl 172
loc 537
rs 3.6
c 0
b 0
f 0

14 Methods

Rating   Name   Duplication   Size   Complexity  
A getEntityManager() 0 4 1
A doIndexAction() 0 16 1
A doExportAction() 0 14 2
C doAddAction() 43 83 12
F doEditAction() 47 115 18
A isLockableEntityLocked() 0 10 2
A doViewAction() 0 27 4
B doDeleteAction() 0 42 7
A doMoveUpAction() 35 35 3
A doMoveDownAction() 35 35 3
A getMaxSortableField() 0 9 1
A setSortWeightOnNewItem() 0 12 2
A buildSortableFieldActions() 12 25 2
A getAdminListRepositoryName() 0 12 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 AdminListController 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 AdminListController, and based on these observations, apply Extract Interface, too.

1
<?php
2
3
namespace Kunstmaan\AdminListBundle\Controller;
4
5
use Doctrine\ORM\EntityManager;
6
use Kunstmaan\AdminBundle\Entity\EntityInterface;
7
use Kunstmaan\AdminBundle\Event\AdaptSimpleFormEvent;
8
use Kunstmaan\AdminBundle\Event\Events;
9
use Kunstmaan\AdminBundle\FlashMessages\FlashTypes;
10
use Kunstmaan\AdminListBundle\AdminList\AdminList;
11
use Kunstmaan\AdminListBundle\AdminList\Configurator\AbstractAdminListConfigurator;
12
use Kunstmaan\AdminListBundle\AdminList\ItemAction\SimpleItemAction;
13
use Kunstmaan\AdminListBundle\AdminList\SortableInterface;
14
use Kunstmaan\AdminListBundle\Entity\LockableEntityInterface;
15
use Kunstmaan\AdminListBundle\Event\AdminListEvent;
16
use Kunstmaan\AdminListBundle\Event\AdminListEvents;
17
use Kunstmaan\NodeBundle\Entity\HasNodeInterface;
18
use Kunstmaan\NodeBundle\Entity\NodeTranslation;
19
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
20
use Symfony\Component\HttpFoundation\RedirectResponse;
21
use Kunstmaan\AdminListBundle\Service\EntityVersionLockService;
22
use Symfony\Component\HttpFoundation\Request;
23
use Symfony\Component\HttpFoundation\Response;
24
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
25
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
26
use Symfony\Component\PropertyAccess\PropertyAccess;
27
28
/**
29
 * AdminListController
30
 */
31
abstract class AdminListController extends Controller
32
{
33
    /**
34
     * You can override this method to return the correct entity manager when using multiple databases ...
35
     *
36
     * @return \Doctrine\Common\Persistence\ObjectManager|object
0 ignored issues
show
Documentation introduced by Wim Vandersmissen
Consider making the return type a bit more specific; maybe use \Doctrine\Common\Persistence\ObjectManager.

This check looks for the generic type array as a return type and suggests a more specific type. This type is inferred from the actual code.

Loading history...
37
     */
38
    protected function getEntityManager()
39
    {
40
        return $this->getDoctrine()->getManager();
41
    }
42
43
    /**
44
     * Shows the list of entities
45
     *
46
     * @param AbstractAdminListConfigurator $configurator
47
     * @param null|Request                  $request
0 ignored issues
show
Documentation introduced by Sander Goossens
Consider making the type for parameter $request a bit more specific; maybe use Request.
Loading history...
48
     *
49
     * @return Response
50
     */
51
    protected function doIndexAction(AbstractAdminListConfigurator $configurator, Request $request)
52
    {
53
        $em = $this->getEntityManager();
54
        /* @var AdminList $adminList */
55
        $adminList = $this->container->get('kunstmaan_adminlist.factory')->createList($configurator, $em);
56
        $adminList->bindRequest($request);
57
58
        $this->buildSortableFieldActions($configurator);
59
60
        return new Response(
61
            $this->renderView(
62
                $configurator->getListTemplate(),
63
                array('adminlist' => $adminList, 'adminlistconfigurator' => $configurator, 'addparams' => array())
64
            )
65
        );
66
    }
67
68
    /**
69
     * Export a list of Entities
70
     *
71
     * @param AbstractAdminListConfigurator $configurator The adminlist configurator
72
     * @param string                        $_format      The format to export to
73
     * @param null|Request                  $request
74
     *
75
     * @throws AccessDeniedHttpException
76
     *
77
     * @return Response
78
     */
79
    protected function doExportAction(AbstractAdminListConfigurator $configurator, $_format, Request $request = null)
80
    {
81
        if (!$configurator->canExport()) {
82
            throw $this->createAccessDeniedException('You do not have sufficient rights to access this page.');
83
        }
84
85
        $em = $this->getEntityManager();
86
87
        /* @var AdminList $adminList */
88
        $adminList = $this->container->get('kunstmaan_adminlist.factory')->createExportList($configurator, $em);
89
        $adminList->bindRequest($request);
0 ignored issues
show
Bug introduced by Kim Ausloos
It seems like $request defined by parameter $request on line 79 can be null; however, Kunstmaan\AdminListBundl...dminList::bindRequest() does not accept null, maybe add an additional type check?

It seems like you allow that null is being passed for a parameter, however the function which is called does not seem to accept null.

We recommend to add an additional type check (or disallow null for the parameter):

function notNullable(stdClass $x) { }

// Unsafe
function withoutCheck(stdClass $x = null) {
    notNullable($x);
}

// Safe - Alternative 1: Adding Additional Type-Check
function withCheck(stdClass $x = null) {
    if ($x instanceof stdClass) {
        notNullable($x);
    }
}

// Safe - Alternative 2: Changing Parameter
function withNonNullableParam(stdClass $x) {
    notNullable($x);
}
Loading history...
90
91
        return $this->container->get('kunstmaan_adminlist.service.export')->getDownloadableResponse($adminList, $_format);
92
    }
93
94
    /**
95
     * Creates and processes the form to add a new Entity
96
     *
97
     * @param AbstractAdminListConfigurator $configurator The adminlist configurator
98
     * @param string                        $type         The type to add
0 ignored issues
show
Documentation introduced by Sander Goossens
Should the type for parameter $type not be string|null?

This check looks for @param annotations where the type inferred by our type inference engine differs from the declared type.

It makes a suggestion as to what type it considers more descriptive.

Most often this is a case of a parameter that can be null in addition to its declared types.

Loading history...
99
     * @param null|Request                  $request
0 ignored issues
show
Documentation introduced by Sander Goossens
Consider making the type for parameter $request a bit more specific; maybe use Request.
Loading history...
100
     *
101
     * @throws AccessDeniedHttpException
102
     *
103
     * @return Response
104
     */
105
    protected function doAddAction(AbstractAdminListConfigurator $configurator, $type = null, Request $request)
106
    {
107
        if (!$configurator->canAdd()) {
108
            throw $this->createAccessDeniedException('You do not have sufficient rights to access this page.');
109
        }
110
111
        /* @var EntityManager $em */
112
        $em = $this->getEntityManager();
113
        $entityName = null;
0 ignored issues
show
Unused Code introduced by Kris Pypen
$entityName is not used, you could remove the assignment.

This check looks for variable assignements that are either overwritten by other assignments or where the variable is not used subsequently.

$myVar = 'Value';
$higher = false;

if (rand(1, 6) > 3) {
    $higher = true;
} else {
    $higher = false;
}

Both the $myVar assignment in line 1 and the $higher assignment in line 2 are dead. The first because $myVar is never used and the second because $higher is always overwritten for every possible time line.

Loading history...
114
        $entityName = (isset($type)) ? $type : $configurator->getRepositoryName();
115
116
        $classMetaData = $em->getClassMetadata($entityName);
117
        // Creates a new instance of the mapped class, without invoking the constructor.
118
        $classname = $classMetaData->getName();
119
        $helper = new $classname();
120
        $helper = $configurator->decorateNewEntity($helper);
121
122
        $formType = $configurator->getAdminType($helper);
123
124
        $event = new AdaptSimpleFormEvent($request, $formType, $helper, $configurator->getAdminTypeOptions());
125
        $event = $this->container->get('event_dispatcher')->dispatch(Events::ADAPT_SIMPLE_FORM, $event);
126
        $tabPane = $event->getTabPane();
127
128
        $form = $this->createForm($formType, $helper, $event->getOptions());
129
130 View Code Duplication
        if ($request->isMethod('POST')) {
0 ignored issues
show
Duplication introduced by Wim Vandersmissen
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...
131
            if ($tabPane) {
132
                $tabPane->bindRequest($request);
133
                $form = $tabPane->getForm();
134
            } else {
135
                $form->handleRequest($request);
136
            }
137
138
            // Don't redirect to listing when coming from ajax request, needed for url chooser.
139
            if ($form->isSubmitted() && $form->isValid() && !$request->isXmlHttpRequest()) {
140
                $adminListEvent = new AdminListEvent($helper, $request, $form);
141
                $this->container->get('event_dispatcher')->dispatch(
142
                    AdminListEvents::PRE_ADD,
143
                    $adminListEvent
144
                );
145
146
                // Check if Response is given
147
                if ($adminListEvent->getResponse() instanceof Response) {
148
                    return $adminListEvent->getResponse();
149
                }
150
151
                // Set sort weight
152
                $helper = $this->setSortWeightOnNewItem($configurator, $helper);
153
154
                $em->persist($helper);
155
                $em->flush();
156
                $this->container->get('event_dispatcher')->dispatch(
157
                    AdminListEvents::POST_ADD,
158
                    $adminListEvent
159
                );
160
161
                // Check if Response is given
162
                if ($adminListEvent->getResponse() instanceof Response) {
163
                    return $adminListEvent->getResponse();
164
                }
165
166
                $indexUrl = $configurator->getIndexUrl();
167
168
                return new RedirectResponse(
169
                    $this->generateUrl($indexUrl['path'], isset($indexUrl['params']) ? $indexUrl['params'] : array())
170
                );
171
            }
172
        }
173
174
        $params = [
175
            'form' => $form->createView(),
176
            'adminlistconfigurator' => $configurator,
177
            'entityVersionLockCheck' => false,
178
        ];
179
180
        if ($tabPane) {
181
            $params = array_merge($params, array('tabPane' => $tabPane));
182
        }
183
184
        return new Response(
185
            $this->renderView($configurator->getAddTemplate(), $params)
186
        );
187
    }
188
189
    /**
190
     * Creates and processes the edit form for an Entity using its ID
191
     *
192
     * @param AbstractAdminListConfigurator $configurator The adminlist configurator
193
     * @param string                        $entityId     The id of the entity that will be edited
194
     * @param null|Request                  $request
0 ignored issues
show
Documentation introduced by Sander Goossens
Consider making the type for parameter $request a bit more specific; maybe use Request.
Loading history...
195
     *
196
     * @throws NotFoundHttpException
197
     * @throws AccessDeniedHttpException
198
     *
199
     * @return Response
200
     */
201
    protected function doEditAction(AbstractAdminListConfigurator $configurator, $entityId, Request $request)
202
    {
203
        /* @var EntityManager $em */
204
        $em = $this->getEntityManager();
205
        $helper = $em->getRepository($configurator->getRepositoryName())->findOneById($entityId);
0 ignored issues
show
Bug introduced by Daan Poron
The method findOneById() does not exist on Doctrine\Common\Persistence\ObjectRepository. Did you maybe mean findOneBy()?

This check marks calls to methods that do not seem to exist on an object.

This is most likely the result of a method being renamed without all references to it being renamed likewise.

Loading history...
206
207
        if ($helper === null) {
208
            throw new NotFoundHttpException('Entity not found.');
209
        }
210
211
        if (!$configurator->canEdit($helper)) {
212
            throw $this->createAccessDeniedException('You do not have sufficient rights to access this page.');
213
        }
214
215
        if ($helper instanceof LockableEntityInterface) {
216
            // This entity is locked
217
            if ($this->isLockableEntityLocked($helper)) {
218
                $indexUrl = $configurator->getIndexUrl();
219
                // Don't redirect to listing when coming from ajax request, needed for url chooser.
220
                if (!$request->isXmlHttpRequest()) {
221
                    /** @var EntityVersionLockService $entityVersionLockService */
222
                    $entityVersionLockService = $this->container->get('kunstmaan_entity.admin_entity.entity_version_lock_service');
223
224
                    $user = $entityVersionLockService->getUsersWithEntityVersionLock($helper, $this->getUser());
225
                    $message = $this->container->get('translator')->trans('kuma_admin_list.edit.flash.locked', array('%user%' => implode(', ', $user)));
226
                    $this->addFlash(
227
                        FlashTypes::WARNING,
228
                        $message
229
                    );
230
231
                    return new RedirectResponse(
232
                        $this->generateUrl(
233
                            $indexUrl['path'],
234
                            isset($indexUrl['params']) ? $indexUrl['params'] : array()
235
                        )
236
                    );
237
                }
238
            }
239
        }
240
241
        $formType = $configurator->getAdminType($helper);
242
243
        $event = new AdaptSimpleFormEvent($request, $formType, $helper, $configurator->getAdminTypeOptions());
244
        $event = $this->container->get('event_dispatcher')->dispatch(Events::ADAPT_SIMPLE_FORM, $event);
245
        $tabPane = $event->getTabPane();
246
247
        $form = $this->createForm($formType, $helper, $event->getOptions());
248
249 View Code Duplication
        if ($request->isMethod('POST')) {
0 ignored issues
show
Duplication introduced by Wim Vandersmissen
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...
250
            if ($tabPane) {
251
                $tabPane->bindRequest($request);
252
                $form = $tabPane->getForm();
253
            } else {
254
                $form->handleRequest($request);
255
            }
256
257
            // Don't redirect to listing when coming from ajax request, needed for url chooser.
258
            if ($form->isSubmitted() && $form->isValid() && !$request->isXmlHttpRequest()) {
259
                $adminListEvent = new AdminListEvent($helper, $request, $form);
260
                $this->container->get('event_dispatcher')->dispatch(
261
                    AdminListEvents::PRE_EDIT,
262
                    $adminListEvent
263
                );
264
265
                // Check if Response is given
266
                if ($adminListEvent->getResponse() instanceof Response) {
267
                    return $adminListEvent->getResponse();
268
                }
269
270
                $em->persist($helper);
271
                $em->flush();
272
                $this->container->get('event_dispatcher')->dispatch(
273
                    AdminListEvents::POST_EDIT,
274
                    $adminListEvent
275
                );
276
277
                // Check if Response is given
278
                if ($adminListEvent->getResponse() instanceof Response) {
279
                    return $adminListEvent->getResponse();
280
                }
281
282
                $indexUrl = $configurator->getIndexUrl();
283
284
                // Don't redirect to listing when coming from ajax request, needed for url chooser.
285
                if (!$request->isXmlHttpRequest()) {
286
                    return new RedirectResponse(
287
                        $this->generateUrl(
288
                            $indexUrl['path'],
289
                            isset($indexUrl['params']) ? $indexUrl['params'] : array()
290
                        )
291
                    );
292
                }
293
            }
294
        }
295
296
        $configurator->buildItemActions();
297
298
        $params = [
299
            'form' => $form->createView(),
300
            'entity' => $helper, 'adminlistconfigurator' => $configurator,
301
            'entityVersionLockInterval' => $this->container->getParameter('kunstmaan_entity.lock_check_interval'),
302
            'entityVersionLockCheck' => $this->container->getParameter('kunstmaan_entity.lock_enabled') && $helper instanceof LockableEntityInterface,
303
        ];
304
305
        if ($tabPane) {
306
            $params = array_merge($params, array('tabPane' => $tabPane));
307
        }
308
309
        return new Response(
310
            $this->renderView(
311
                $configurator->getEditTemplate(),
312
                $params
313
            )
314
        );
315
    }
316
317
    protected function doViewAction(AbstractAdminListConfigurator $configurator, $entityId, Request $request)
318
    {
319
        /* @var EntityManager $em */
320
        $em = $this->getEntityManager();
321
        $helper = $em->getRepository($configurator->getRepositoryName())->findOneById($entityId);
0 ignored issues
show
Bug introduced by whitewhidow
The method findOneById() does not exist on Doctrine\Common\Persistence\ObjectRepository. Did you maybe mean findOneBy()?

This check marks calls to methods that do not seem to exist on an object.

This is most likely the result of a method being renamed without all references to it being renamed likewise.

Loading history...
322
        if ($helper === null) {
323
            throw new NotFoundHttpException('Entity not found.');
324
        }
325
326
        if (!$configurator->canView($helper)) {
327
            throw $this->createAccessDeniedException('You do not have sufficient rights to access this page.');
328
        }
329
330
        $MetaData = $em->getClassMetadata($configurator->getRepositoryName());
331
        $fields = array();
332
        $accessor = PropertyAccess::createPropertyAccessor();
333
        foreach ($MetaData->fieldNames as $value) {
334
            $fields[$value] = $accessor->getValue($helper, $value);
335
        }
336
337
        return new Response(
338
            $this->renderView(
339
                $configurator->getViewTemplate(),
340
                array('entity' => $helper, 'adminlistconfigurator' => $configurator, 'fields' => $fields)
341
            )
342
        );
343
    }
344
345
    /**
346
     * Delete the Entity using its ID
347
     *
348
     * @param AbstractAdminListConfigurator $configurator The adminlist configurator
349
     * @param int                           $entityId     The id to delete
350
     * @param null|Request                  $request
0 ignored issues
show
Documentation introduced by Sander Goossens
Consider making the type for parameter $request a bit more specific; maybe use Request.
Loading history...
351
     *
352
     * @throws NotFoundHttpException
353
     * @throws AccessDeniedHttpException
354
     *
355
     * @return Response
356
     */
357
    protected function doDeleteAction(AbstractAdminListConfigurator $configurator, $entityId, Request $request)
358
    {
359
        /* @var $em EntityManager */
360
        $em = $this->getEntityManager();
361
        $helper = $em->getRepository($configurator->getRepositoryName())->findOneById($entityId);
0 ignored issues
show
Bug introduced by Daan Poron
The method findOneById() does not exist on Doctrine\Common\Persistence\ObjectRepository. Did you maybe mean findOneBy()?

This check marks calls to methods that do not seem to exist on an object.

This is most likely the result of a method being renamed without all references to it being renamed likewise.

Loading history...
362
        if ($helper === null) {
363
            throw new NotFoundHttpException('Entity not found.');
364
        }
365
        if (!$configurator->canDelete($helper)) {
366
            throw $this->createAccessDeniedException('You do not have sufficient rights to access this page.');
367
        }
368
369
        $indexUrl = $configurator->getIndexUrl();
370
        if ($request->isMethod('POST')) {
371
            $adminListEvent = new AdminListEvent($helper, $request);
372
            $this->container->get('event_dispatcher')->dispatch(
373
                AdminListEvents::PRE_DELETE,
374
                $adminListEvent
375
            );
376
377
            // Check if Response is given
378
            if ($adminListEvent->getResponse() instanceof Response) {
379
                return $adminListEvent->getResponse();
380
            }
381
382
            $em->remove($helper);
383
            $em->flush();
384
            $this->container->get('event_dispatcher')->dispatch(
385
                AdminListEvents::POST_DELETE,
386
                $adminListEvent
387
            );
388
389
            // Check if Response is given
390
            if ($adminListEvent->getResponse() instanceof Response) {
391
                return $adminListEvent->getResponse();
392
            }
393
        }
394
395
        return new RedirectResponse(
396
            $this->generateUrl($indexUrl['path'], isset($indexUrl['params']) ? $indexUrl['params'] : array())
397
        );
398
    }
399
400
    /**
401
     * Move an item up in the list.
402
     *
403
     * @return RedirectResponse
404
     */
405 View Code Duplication
    protected function doMoveUpAction(AbstractAdminListConfigurator $configurator, $entityId, Request $request)
0 ignored issues
show
Duplication introduced by Sander Goossens
This method seems to be duplicated in 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...
406
    {
407
        $em = $this->getEntityManager();
408
        $sortableField = $configurator->getSortableField();
0 ignored issues
show
Bug introduced by Sander Goossens
It seems like you code against a specific sub-type and not the parent class Kunstmaan\AdminListBundl...ctAdminListConfigurator as the method getSortableField() does only exist in the following sub-classes of Kunstmaan\AdminListBundl...ctAdminListConfigurator: Kunstmaan\AdminListBundl...nList\Configurator\DBAL, Kunstmaan\AdminListBundl...inList\Configurator\ORM. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

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

class MyUser extends 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 sub-classes 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 parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
409
410
        $repositoryName = $this->getAdminListRepositoryName($configurator);
411
412
        $repo = $em->getRepository($repositoryName);
413
        $item = $repo->find($entityId);
414
415
        $setter = 'set'.ucfirst($sortableField);
416
        $getter = 'get'.ucfirst($sortableField);
417
418
        $nextItem = $repo->createQueryBuilder('i')
419
            ->where('i.'.$sortableField.' < :weight')
420
            ->setParameter('weight', $item->$getter())
421
            ->orderBy('i.'.$sortableField, 'DESC')
422
            ->setMaxResults(1)
423
            ->getQuery()
424
            ->getOneOrNullResult();
425
        if ($nextItem) {
426
            $nextItem->$setter($item->$getter());
427
            $em->persist($nextItem);
428
            $item->$setter($item->$getter() - 1);
429
430
            $em->persist($item);
0 ignored issues
show
Bug introduced by Krisztián Ferenczi
It seems like $item defined by $repo->find($entityId) on line 413 can also be of type null; however, Doctrine\Common\Persiste...bjectManager::persist() does only seem to accept object, maybe add an additional type check?

If a method or function can return multiple different values and unless you are sure that you only can receive a single value in this context, we recommend to add an additional type check:

/**
 * @return array|string
 */
function returnsDifferentValues($x) {
    if ($x) {
        return 'foo';
    }

    return array();
}

$x = returnsDifferentValues($y);
if (is_array($x)) {
    // $x is an array.
}

If this a common case that PHP Analyzer should handle natively, please let us know by opening an issue.

Loading history...
431
            $em->flush();
432
        }
433
434
        $indexUrl = $configurator->getIndexUrl();
435
436
        return new RedirectResponse(
437
            $this->generateUrl($indexUrl['path'], isset($indexUrl['params']) ? $indexUrl['params'] : array())
438
        );
439
    }
440
441 View Code Duplication
    protected function doMoveDownAction(AbstractAdminListConfigurator $configurator, $entityId, Request $request)
0 ignored issues
show
Duplication introduced by Sander Goossens
This method seems to be duplicated in 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...
442
    {
443
        $em = $this->getEntityManager();
444
        $sortableField = $configurator->getSortableField();
0 ignored issues
show
Bug introduced by Sander Goossens
It seems like you code against a specific sub-type and not the parent class Kunstmaan\AdminListBundl...ctAdminListConfigurator as the method getSortableField() does only exist in the following sub-classes of Kunstmaan\AdminListBundl...ctAdminListConfigurator: Kunstmaan\AdminListBundl...nList\Configurator\DBAL, Kunstmaan\AdminListBundl...inList\Configurator\ORM. Maybe you want to instanceof check for one of these explicitly?

Let’s take a look at an example:

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

class MyUser extends 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 sub-classes 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 parent class:

    abstract class User
    {
        /** @return string */
        abstract public function getPassword();
    
        /** @return string */
        abstract public function getDisplayName();
    }
    
Loading history...
445
446
        $repositoryName = $this->getAdminListRepositoryName($configurator);
447
448
        $repo = $em->getRepository($repositoryName);
449
        $item = $repo->find($entityId);
450
451
        $setter = 'set'.ucfirst($sortableField);
452
        $getter = 'get'.ucfirst($sortableField);
453
454
        $nextItem = $repo->createQueryBuilder('i')
455
            ->where('i.'.$sortableField.' > :weight')
456
            ->setParameter('weight', $item->$getter())
457
            ->orderBy('i.'.$sortableField, 'ASC')
458
            ->setMaxResults(1)
459
            ->getQuery()
460
            ->getOneOrNullResult();
461
        if ($nextItem) {
462
            $nextItem->$setter($item->$getter());
463
            $em->persist($nextItem);
464
            $item->$setter($item->$getter() + 1);
465
466
            $em->persist($item);
0 ignored issues
show
Bug introduced by Krisztián Ferenczi
It seems like $item defined by $repo->find($entityId) on line 449 can also be of type null; however, Doctrine\Common\Persiste...bjectManager::persist() does only seem to accept object, maybe add an additional type check?

If a method or function can return multiple different values and unless you are sure that you only can receive a single value in this context, we recommend to add an additional type check:

/**
 * @return array|string
 */
function returnsDifferentValues($x) {
    if ($x) {
        return 'foo';
    }

    return array();
}

$x = returnsDifferentValues($y);
if (is_array($x)) {
    // $x is an array.
}

If this a common case that PHP Analyzer should handle natively, please let us know by opening an issue.

Loading history...
467
            $em->flush();
468
        }
469
470
        $indexUrl = $configurator->getIndexUrl();
471
472
        return new RedirectResponse(
473
            $this->generateUrl($indexUrl['path'], isset($indexUrl['params']) ? $indexUrl['params'] : array())
474
        );
475
    }
476
477
    private function getMaxSortableField($repo, $sort)
478
    {
479
        $maxWeight = $repo->createQueryBuilder('i')
480
            ->select('max(i.'.$sort.')')
481
            ->getQuery()
482
            ->getSingleScalarResult();
483
484
        return (int) $maxWeight;
485
    }
486
487
    /**
488
     * @param LockableEntityInterface $entity
489
     *
490
     * @return bool
491
     */
492
    protected function isLockableEntityLocked(LockableEntityInterface $entity)
493
    {
494
        /** @var EntityVersionLockService $entityVersionLockService */
495
        $entityVersionLockService = $this->container->get('kunstmaan_entity.admin_entity.entity_version_lock_service');
496
497
        return $entityVersionLockService->isEntityBelowThreshold($entity) && $entityVersionLockService->isEntityLocked(
498
                $this->getUser(),
0 ignored issues
show
Documentation introduced by Ruud Denivel
$this->getUser() is of type null|object, but the function expects a object<FOS\UserBundle\Model\User>.

It seems like the type of the argument is not accepted by the function/method which you are calling.

In some cases, in particular if PHP’s automatic type-juggling kicks in this might be fine. In other cases, however this might be a bug.

We suggest to add an explicit type cast like in the following example:

function acceptsInteger($int) { }

$x = '123'; // string "123"

// Instead of
acceptsInteger($x);

// we recommend to use
acceptsInteger((integer) $x);
Loading history...
499
                $entity
500
            );
501
    }
502
503
    /**
504
     * Sets the sort weight on a new item. Can be overridden if a non-default sorting implementation is being used.
505
     *
506
     * @param AbstractAdminListConfigurator $configurator The adminlist configurator
507
     * @param $item
508
     *
509
     * @return mixed
510
     */
511
    protected function setSortWeightOnNewItem(AbstractAdminListConfigurator $configurator, $item)
512
    {
513
        if ($configurator instanceof SortableInterface) {
514
            $repo = $this->getEntityManager()->getRepository($configurator->getRepositoryName());
515
            $sort = $configurator->getSortableField();
516
            $weight = $this->getMaxSortableField($repo, $sort);
517
            $setter = 'set'.ucfirst($sort);
518
            $item->$setter($weight + 1);
519
        }
520
521
        return $item;
522
    }
523
524
    protected function buildSortableFieldActions(AbstractAdminListConfigurator $configurator)
525
    {
526
        // Check if Sortable interface is implemented
527
        if ($configurator instanceof SortableInterface) {
528 View Code Duplication
            $route = function (EntityInterface $item) use ($configurator) {
0 ignored issues
show
Duplication introduced by Mattias Delang
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...
529
                return array(
530
                    'path' => $configurator->getPathByConvention().'_move_up',
531
                    'params' => array('id' => $item->getId()),
532
                );
533
            };
534
535
            $action = new SimpleItemAction($route, 'arrow-up', 'kuma_admin_list.action.move_up');
536
            $configurator->addItemAction($action);
537
538 View Code Duplication
            $route = function (EntityInterface $item) use ($configurator) {
0 ignored issues
show
Duplication introduced by Mattias Delang
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...
539
                return array(
540
                    'path' => $configurator->getPathByConvention().'_move_down',
541
                    'params' => array('id' => $item->getId()),
542
                );
543
            };
544
545
            $action = new SimpleItemAction($route, 'arrow-down', 'kuma_admin_list.action.move_down');
546
            $configurator->addItemAction($action);
547
        }
548
    }
549
550
    /**
551
     * @param AbstractAdminListConfigurator $configurator
552
     *
553
     * @return string
554
     */
555
    protected function getAdminListRepositoryName(AbstractAdminListConfigurator $configurator)
556
    {
557
        $em = $this->getEntityManager();
558
        $className = $em->getClassMetadata($configurator->getRepositoryName())->getName();
559
560
        $implements = class_implements($className);
561
        if (isset($implements[HasNodeInterface::class])) {
562
            return NodeTranslation::class;
563
        }
564
565
        return $configurator->getRepositoryName();
566
    }
567
}
568