Completed
Pull Request — master (#336)
by Leny
06:08
created

BasePageController::translateAction()   B

Complexity

Conditions 8
Paths 26

Size

Total Lines 60
Code Lines 39

Duplication

Lines 0
Ratio 0 %

Importance

Changes 5
Bugs 0 Features 0
Metric Value
c 5
b 0
f 0
dl 0
loc 60
rs 7.0677
cc 8
eloc 39
nc 26
nop 2

How to fix   Long Method   

Long Method

Small methods make your code easier to understand, in particular if combined with a good name. Besides, if your method is small, finding a good name is usually much easier.

For example, if you find yourself adding comments to a method's body, this is usually a good sign to extract the commented part to a new method, and use the comment as a starting point when coming up with a good name for this new method.

Commonly applied refactorings include:

1
<?php
2
3
namespace Victoire\Bundle\PageBundle\Controller;
4
5
use Symfony\Bundle\FrameworkBundle\Controller\Controller;
6
use Symfony\Component\HttpFoundation\Request;
7
use Symfony\Component\HttpFoundation\Response;
8
use Victoire\Bundle\BusinessPageBundle\Entity\BusinessPage;
9
use Victoire\Bundle\BusinessPageBundle\Entity\BusinessTemplate;
10
use Victoire\Bundle\CoreBundle\Controller\VictoireAlertifyControllerTrait;
11
use Victoire\Bundle\CoreBundle\Entity\WebViewInterface;
12
use Victoire\Bundle\PageBundle\Entity\BasePage;
13
use Victoire\Bundle\PageBundle\Entity\Page;
14
use Victoire\Bundle\TemplateBundle\Entity\Template;
15
use Victoire\Bundle\ViewReferenceBundle\ViewReference\ViewReference;
16
17
/**
18
 * The base page controller is used to interact with all kind of pages.
19
 **/
20
class BasePageController extends Controller
21
{
22
    use VictoireAlertifyControllerTrait;
23
24
    public function showAction(Request $request, $url = '')
25
    {
26
        $response = $this->get('victoire_page.page_helper')->renderPageByUrl(
27
            $url,
28
            $request->getLocale(),
29
            $request->isXmlHttpRequest()
30
        );
31
32
        //throw an exception is the page is not valid
33
        return $response;
34
    }
35
36
    public function showByIdAction(Request $request, $viewId, $entityId = null)
37
    {
38
        $parameters = ['viewId' => $viewId];
39
        if ($entityId) {
40
            $parameters['entityId'] = $entityId;
41
        }
42
        $page = $this->get('victoire_page.page_helper')->findPageByParameters($parameters);
43
44
        return $this->redirect($this->generateUrl('victoire_core_page_show', array_merge(
45
                ['url' => $page->getReference()->getUrl()],
46
                $request->query->all()
47
            )
48
        ));
49
    }
50
51
    public function showBusinessPageByIdAction($entityId, $type)
52
    {
53
        $businessEntityHelper = $this->get('victoire_core.helper.queriable_business_entity_helper');
54
        $businessEntity = $businessEntityHelper->findById($type);
55
        $entity = $businessEntityHelper->getByBusinessEntityAndId($businessEntity, $entityId);
56
57
        $refClass = new \ReflectionClass($entity);
58
59
        $templateId = $this->get('victoire_business_page.business_page_helper')
60
            ->guessBestPatternIdForEntity($refClass, $entityId, $this->container->get('doctrine.orm.entity_manager'));
61
62
        $page = $this->get('victoire_page.page_helper')->findPageByParameters([
63
            'viewId'   => $templateId,
64
            'entityId' => $entityId,
65
        ]);
66
        $this->get('victoire_widget_map.builder')->build($page);
67
        $this->get('victoire_widget_map.widget_data_warmer')->warm(
68
            $this->get('doctrine.orm.entity_manager'),
69
            $page
70
        );
71
72
        return $this->redirect(
73
            $this->generateUrl(
74
                'victoire_core_page_show',
75
                [
76
                    'url' => $page->getReference()->Url(),
77
                ]
78
            )
79
        );
80
    }
81
82
    /**
83
     * New page.
84
     *
85
     * @param bool $isHomepage
86
     *
87
     * @return []
0 ignored issues
show
Documentation introduced by
The doc-type [] could not be parsed: Unknown type name "" at position 0. [(view supported doc-types)

This check marks PHPDoc comments that could not be parsed by our parser. To see which comment annotations we can parse, please refer to our documentation on supported doc-types.

Loading history...
88
     */
89
    protected function newAction($isHomepage = false)
90
    {
91
        $entityManager = $this->get('doctrine.orm.entity_manager');
92
        $page = $this->getNewPage();
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Victoire\Bundle\PageBund...ller\BasePageController as the method getNewPage() does only exist in the following sub-classes of Victoire\Bundle\PageBund...ller\BasePageController: Victoire\Bundle\BlogBund...ntroller\BlogController, Victoire\Bundle\PageBund...dministrationController. 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...
93
        if ($page instanceof Page) {
94
            $page->setHomepage($isHomepage ? $isHomepage : 0);
0 ignored issues
show
Bug introduced by
It seems like $isHomepage ? $isHomepage : 0 can also be of type integer; however, Victoire\Bundle\PageBund...iewTrait::setHomepage() does only seem to accept boolean, 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...
95
        }
96
97
        $form = $this->get('form.factory')->create($this->getNewPageType(), $page);
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Victoire\Bundle\PageBund...ller\BasePageController as the method getNewPageType() does only exist in the following sub-classes of Victoire\Bundle\PageBund...ller\BasePageController: Victoire\Bundle\BlogBund...ntroller\BlogController, Victoire\Bundle\PageBund...dministrationController. 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...
98
99
        $form->handleRequest($this->get('request'));
100
        if ($form->isValid()) {
101
            if ($page->getParent()) {
102
                $pageNb = count($page->getParent()->getChildren());
103
            } else {
104
                $pageNb = count($entityManager->getRepository('VictoirePageBundle:BasePage')->findByParent(null));
105
            }
106
            // + 1 because position start at 1, not 0
107
            $page->setPosition($pageNb + 1);
108
109
            $page->setAuthor($this->getUser());
110
            $entityManager->persist($page);
111
            $entityManager->flush();
112
113
            // If the $page is a BusinessEntity (eg. an Article), compute it's url
114
            if (null !== $this->get('victoire_core.helper.business_entity_helper')->findByEntityInstance($page)) {
115
                $page = $this
116
                        ->get('victoire_business_page.business_page_builder')
117
                        ->generateEntityPageFromTemplate($page->getTemplate(), $page, $entityManager);
118
            }
119
120
            $this->congrat($this->get('translator')->trans('victoire_page.create.success', [], 'victoire'));
121
            $viewReference = $this->get('victoire_view_reference.repository')->getOneReferenceByParameters([
122
                'viewId' => $page->getId(),
123
            ]);
124
125
            return [
126
                'success'  => true,
127
                'url'      => $this->generateUrl(
128
                    'victoire_core_page_show',
129
                    [
130
                        '_locale' => $page->getLocale(),
131
                        'url'     => $viewReference->getUrl(),
132
                    ]
133
                ),
134
            ];
135
        } else {
136
            $formErrorHelper = $this->get('victoire_form.error_helper');
137
138
            return [
139
                'success' => false,
140
                'message' => $formErrorHelper->getRecursiveReadableErrors($form),
141
                'html'    => $this->get('victoire_templating')->render(
142
                    $this->getBaseTemplatePath().':new.html.twig',
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Victoire\Bundle\PageBund...ller\BasePageController as the method getBaseTemplatePath() does only exist in the following sub-classes of Victoire\Bundle\PageBund...ller\BasePageController: Victoire\Bundle\BlogBund...ntroller\BlogController, Victoire\Bundle\PageBund...dministrationController. 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...
143
                    ['form' => $form->createView()]
144
                ),
145
            ];
146
        }
147
    }
148
149
    /**
150
     * Page settings.
151
     *
152
     * @param Request  $request
153
     * @param BasePage $page
154
     *
155
     * @return array
156
     */
157
    protected function settingsAction(Request $request, BasePage $page)
158
    {
159
        $entityManager = $this->getDoctrine()->getManager();
160
        $form = $this->createForm($this->getPageSettingsType(), $page);
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Victoire\Bundle\PageBund...ller\BasePageController as the method getPageSettingsType() does only exist in the following sub-classes of Victoire\Bundle\PageBund...ller\BasePageController: Victoire\Bundle\BlogBund...ntroller\BlogController, Victoire\Bundle\PageBund...dministrationController. 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...
161
        $businessProperties = [];
162
163
        //if the page is a business entity page pattern
164
        if ($page instanceof BusinessTemplate) {
165
            //we can use the business entity properties on the seo
166
            $businessEntity = $this->get('victoire_core.helper.business_entity_helper')->findById($page->getBusinessEntityId());
167
            $businessProperties = $businessEntity->getBusinessPropertiesByType('seoable');
168
        }
169
170
        //if the page is a business entity page
171
        if ($page instanceof BusinessPage) {
172
            $form = $this->createForm($this->getBusinessPageType(), $page);
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Victoire\Bundle\PageBund...ller\BasePageController as the method getBusinessPageType() does only exist in the following sub-classes of Victoire\Bundle\PageBund...ller\BasePageController: Victoire\Bundle\PageBund...dministrationController. 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...
173
        }
174
175
        $form->handleRequest($request);
176
177
        if ($form->isValid()) {
178
            $entityManager->persist($page);
179
            $entityManager->flush();
180
            /** @var ViewReference $viewReference */
181
            $viewReference = $this->get('victoire_view_reference.repository')
182
                ->getOneReferenceByParameters(['viewId' => $page->getId()]);
183
184
            $page->setReference($viewReference);
185
186
            $this->congrat($this->get('translator')->trans('victoire_page.update.success', [], 'victoire'));
187
188
            return [
189
                'success' => true,
190
                'url'     => $this->generateUrl(
191
                    'victoire_core_page_show', [
192
                        '_locale' => $page->getLocale(),
193
                        'url'     => $viewReference->getUrl(),
194
                    ]
195
                ),
196
            ];
197
        }
198
        //we display the form
199
        $errors = $this->get('victoire_form.error_helper')->getRecursiveReadableErrors($form);
200
201
        return  [
202
            'success' => empty($errors),
203
            'html'    => $this->get('victoire_templating')->render(
204
                $this->getBaseTemplatePath().':settings.html.twig',
0 ignored issues
show
Bug introduced by
It seems like you code against a specific sub-type and not the parent class Victoire\Bundle\PageBund...ller\BasePageController as the method getBaseTemplatePath() does only exist in the following sub-classes of Victoire\Bundle\PageBund...ller\BasePageController: Victoire\Bundle\BlogBund...ntroller\BlogController, Victoire\Bundle\PageBund...dministrationController. 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...
205
                [
206
                    'page'               => $page,
207
                    'form'               => $form->createView(),
208
                    'businessProperties' => $businessProperties,
209
                ]
210
            ),
211
            'message' => $errors,
212
        ];
213
    }
214
215
    /**
216
     * @param Page $page The page to delete
217
     *
218
     * @return Response
219
     */
220
    public function deleteAction(BasePage $page)
221
    {
222
        $response = null;
0 ignored issues
show
Unused Code introduced by
$response 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...
223
224
        try {
225
            //it should not be allowed to try to delete an undeletable page
226
            if ($page->isUndeletable()) {
227
                $message = $this->get('translator')->trans('page.undeletable', [], 'victoire');
228
                throw new \Exception($message);
229
            }
230
231
            //the entity manager
232
            $entityManager = $this->get('doctrine.orm.entity_manager');
233
234
            //remove the page
235
            $entityManager->remove($page);
236
237
            //flush the modifications
238
            $entityManager->flush();
239
240
            //redirect to the homepage
241
242
            $homepageUrl = $this->generateUrl('victoire_core_homepage_show');
243
244
            $response = [
245
                'success' => true,
246
                'url'     => $homepageUrl,
247
            ];
248
        } catch (\Exception $ex) {
249
            $response = [
250
                'success' => false,
251
                'message' => $ex->getMessage(),
252
            ];
253
        }
254
255
        return $response;
256
    }
257
}
258