PHPStan Level 2

[vv12131415]

Subject

I've bumped PHPStan to level 2. But since we can't break things most of the changes are baseline file and commented code in interfaces.

I am targeting this branch, because this is not BC break.

To do

• Choose what error to thow in src/Controller/CRUDController.php; (no error, just isset check)
• Is that ok for me to override properties only because Phpstan can understand (see CRUDController and *Mapper classes for examples). I think no, but this way PHPStan is thowing errors because base interface (BuilderInterface) doesn't have some methods that are required here (ShowBuilderInterface and it's addField method)
• What to do with other errors that left; (no errors of PHPStan left)

 ------ --------------------------------------------------------------------------------------------- 
  Line   Command/QuestionableCommand.php                                                              
 ------ --------------------------------------------------------------------------------------------- 
  41     Call to an undefined method Symfony\Component\Console\Helper\QuestionHelper::getQuestion().  
  64     Call to an undefined method Symfony\Component\Console\Helper\QuestionHelper::getQuestion().  
 ------ --------------------------------------------------------------------------------------------- 

 ------ --------------------------------------------------------------------------------------------------- 
  Line   Security/Handler/AclSecurityHandler.php                                                            
 ------ --------------------------------------------------------------------------------------------------- 
  191    Call to an undefined method Symfony\Component\Security\Acl\Model\AclInterface::insertObjectAce().  
  213    Call to an undefined method Symfony\Component\Security\Acl\Model\AclInterface::insertClassAce().   
  215    Call to an undefined method Symfony\Component\Security\Acl\Model\AclInterface::updateClassAce().   
  220    Call to an undefined method Symfony\Component\Security\Acl\Model\AclInterface::deleteClassAce().   
 ------ --------------------------------------------------------------------------------------------------- 

 ------ -------------------------------------------------------------------------------------------------- 
  Line   Util/AdminAclManipulator.php                                                                      
 ------ -------------------------------------------------------------------------------------------------- 
  94     Call to an undefined method Symfony\Component\Security\Acl\Model\AclInterface::insertClassAce().  
  97     Call to an undefined method Symfony\Component\Security\Acl\Model\AclInterface::updateClassAce().  
  107    Call to an undefined method Symfony\Component\Security\Acl\Model\AclInterface::deleteClassAce().  
 ------ -------------------------------------------------------------------------------------------------- 

[vv12131415]

this is ready for review/discussion

[VincentLanglet]

In a general way, I think it would be better to avoid writing

/** @var Foo $foo */

And rely instead on

assert($foo instanceof Foo)

Or

if (!$foo instanceof Foo) {
  // do something else or throws an exception
}

Because if $foo HAVE TO be a FOO instance, it's better to check for it when the param could be an more generic Interface instead.

[vv12131415]

Okay. Most of it is fixed. BUT I have those

 ------ --------------------------------------------------------------------------------------------- 
  Line   Command/QuestionableCommand.php                                                              
 ------ --------------------------------------------------------------------------------------------- 
  41     Call to an undefined method Symfony\Component\Console\Helper\QuestionHelper::getQuestion().  
  64     Call to an undefined method Symfony\Component\Console\Helper\QuestionHelper::getQuestion().  
 ------ --------------------------------------------------------------------------------------------- 

what should be done with them?

I can simply add them to baseline.
I haven't found any method like this one, on QuestionHelper::getQuestion

[vv12131415]

Have no idea why phpstan-qa fails with this

 In Resolver.php line 429:
                                                                               
  Service 'rules.59' (type of PHPStan\Rules\Symfony\ContainerInterfacePrivate  
  ServiceRule): Multiple services of type PHPStan\Symfony\ServiceMap found: 0  
  270, 0293 (needed by $symfonyServiceMap in __construct())                    
                                                                               

In Autowiring.php line 50:
                                                                          
  Multiple services of type PHPStan\Symfony\ServiceMap found: 0270, 0293  
                                                                     

any idea?

---

It was phpstan-symfony plugin.
I'm removing it, since it requires container.xml

You have to provide a path to srcDevDebugProjectContainer.xml https://github.com/phpstan/phpstan-symfony#configuration

and breaks CI build

[VincentLanglet]

Okay. Most of it is fixed. BUT I have those

 ------ --------------------------------------------------------------------------------------------- 
  Line   Command/QuestionableCommand.php                                                              
 ------ --------------------------------------------------------------------------------------------- 
  41     Call to an undefined method Symfony\Component\Console\Helper\QuestionHelper::getQuestion().  
  64     Call to an undefined method Symfony\Component\Console\Helper\QuestionHelper::getQuestion().  
 ------ --------------------------------------------------------------------------------------------- 

what should be done with them?

I can simply add them to baseline.
I haven't found any method like this one, on QuestionHelper::getQuestion

I would change

new Question($questionHelper->getQuestion($questionText, $default), $default);

to

new Question($questionText, $default);

And deprecate the getQuestionHelper method.

I don't know how it could currently work.

[vv12131415]

And deprecate the getQuestionHelper method.

Can't deprecate it, since we need to QuestionHelper::ask()

[vv12131415]

can someone help with question
I don't get failures in tests when calling assert I even tried something like this
php -d zend.assertions=-1 -d assert.exception=1 vendor/bin/simple-phpunit -c phpunit.xml.dist tests/Security/Handler/AclSecurityHandlerTest.php

1. How should I configure my local enviroment?
2. How to test against assert?

---

upd: note to self use something like this

php -d zend.assertions=1 -d assert.exception=0 vendor/bin/simple-phpunit -c phpunit.xml.dist

[vv12131415]

@VincentLanglet now it's finished

[phansys]

I noticed there are some new exceptions. Is yet "pedantic" the right label? Don't we need a changelog for these exceptions? Are they respecting BC?

[VincentLanglet]

I noticed there are some new exceptions. Is yet "pedantic" the right label? Don't we need a changelog for these exceptions? Are they respecting BC?

If you now pass by these exception the code wasn't working before.

This situation is always similar to this

$metadata = $this->validator->getMetadataFor($this->getClass());
if (!$metadata instanceof GenericMetadata) {
    throw new \Exception();
}

$metadata->someMethodThatOnlyGenericMetadataHas();

[phansys]

I understand, although I guess a border case could be reached if the previous exception has a different type than the new one and the user is relying on these types.

[VincentLanglet]

I understand, although I guess a border case could be reached if the previous exception has a different type than the new one and the user is relying on these types.

Indeed. Previously this was an error, now it's an exception. But nobody should catch an error. So IMHO this would be such a border case and a wrong usage of these methods, that this shouldn't be under the BC-promises.
We should be able to update the Phpstan level without a new major version.

[phansys]

We should be able to update the Phpstan level without a new major version.

Agree. I'm just saying that an upgrade note should help to cover such edge scenarios in case they are met.

[vv12131415]

@phansys any chance to get this merged?

[phansys]

IMO, since there are new exceptions thrown, an upgrade note should be required.
Regardless that, I think the PR is RTM.

[greg0ire]

Thanks @vladyslavstartsev