phpstan / phpstan-symfony

Symfony extension for PHPStan
MIT License
700 stars 89 forks source link

Controller with multiple has calls in the same nested if #178

Closed jordisala1991 closed 3 years ago

jordisala1991 commented 3 years ago

We are trying to increase Sonata phpstan levels and we found a strange behavior when trying to upgrade to level 5:

https://github.com/sonata-project/SonataMediaBundle/pull/2002#discussion_r661189046

The thing is, when I have the controller with 2 checks on has for a different services on the same if, it fails:

 ------ ----------------------------------------------------------------------------------------------------------------------------------------
  Line   src/Controller/MediaAdminController.php
 ------ ----------------------------------------------------------------------------------------------------------------------------------------
  75     Call to method Symfony\Bundle\FrameworkBundle\Controller\AbstractController::has() with 'sonata.media…' will always evaluate to false.
 ------ ----------------------------------------------------------------------------------------------------------------------------------------

The code is something like this:

        if ($this->has('sonata.media.manager.category') && $this->has('sonata.media.manager.context')) {
            $rootCategories = $this->get('sonata.media.manager.category')->getRootCategoriesForContext(
                $this->get('sonata.media.manager.context')->find($context)
            );

            $rootCategory = current($rootCategories);
        }

If have tried this other approach:

        if ($this->has('sonata.media.manager.category')) {
            $rootCategories = $this->get('sonata.media.manager.category')->getRootCategoriesForContext(
                $this->has('sonata.media.manager.context') ? $this->get('sonata.media.manager.context')->find($context) : null
            );

            $rootCategory = current($rootCategories);
        }

The error is the same, I have tried to also invert the condition, but it fails too. In fact if I invert the condition it starts failing on other parts of the code:

        if ($this->has('sonata.media.manager.context') && $this->has('sonata.media.manager.category')) {
            $rootCategories = $this->get('sonata.media.manager.category')->getRootCategoriesForContext(
                $this->get('sonata.media.manager.context')->find($context)
            );

            $rootCategory = current($rootCategories);
        }

        if (null !== $rootCategory && !$filters) {
            $datagrid->setValue('category', null, $rootCategory->getId());
        }
        if ($this->has('sonata.media.manager.category') && $request->get('category')) {
            $category = $this->get('sonata.media.manager.category')->findOneBy([
                'id' => (int) $request->get('category'),
                'context' => $context,
            ]);

            if (!empty($category)) {
                $datagrid->setValue('category', null, $category->getId());
            } else {
                $datagrid->setValue('category', null, $rootCategory->getId());
            }
        }

Then it fails on the same initial line and on the next has check for the same service.

I tried digging into the code of this plugin but I don't understand enough of phpstan internals to find the problem, but I can provide stack traces or help debuging further if needed.

ondrejmirtes commented 3 years ago

I'm not sure what's wrong, but try taking advantage of the advanced analysis where PHPStan knows what's in your DIC: https://github.com/phpstan/phpstan-symfony#configuration

VincentLanglet commented 3 years ago

Seems related to https://github.com/phpstan/phpstan-symfony#constant-hassers

ondrejmirtes commented 3 years ago

@VincentLanglet I don't think so, they don't have the configuration to read DIC contents yet.

jordisala1991 commented 3 years ago

Disabling constant_hassers does not fix the problem. To be noted that if I split:

if ($this->has('sonata.media.manager.category') && $this->has('sonata.media.manager.context')) {
    // do stuff that requires both managers.
}

into

if ($this->has('sonata.media.manager.category')) {
    // do some stuff that requires manager.category
}
if ($this->has('sonata.media.manager.context')) {
    // do another things that requires manager.context
}

it works (but well my logic won't be correct because I need to check in the same conditional for both services).

ondrejmirtes commented 3 years ago

Fixed: https://github.com/phpstan/phpstan-symfony/commit/58b5959a11e26ce495b424ee634e721aaa9674d1

jordisala1991 commented 3 years ago

Thanks @ondrejmirtes ! :)

github-actions[bot] commented 3 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.