Raise phpstan to max level#1386
Raise phpstan to max level#1386OskarStark merged 3 commits intosonata-project:masterfrom VincentLanglet:phpstanMax
Conversation
| { | ||
| if (null === $type) { | ||
| $guessType = $this->guesser->guess($fieldDescription); | ||
| if (null === $guessType) { |
There was a problem hiding this comment.
If every time we are returning null we throw an exception, wouldnt it be better to only return non null values or throw exception inside the guesser?
There was a problem hiding this comment.
I made sonata-project/SonataAdminBundle#6987 because the TypeGuesserChain could return null...
If you have a TypeGuesserChain with no guesser how can you return a type ?
And the Guesser interface from Symfony was allowing null as return type...
The Guesser from this bundle never return null, but just in case, I added the exception...
| { | ||
| $template = $blockContext->getTemplate(); | ||
| \assert(null !== $template); | ||
| $limit = $blockContext->getSetting('limit'); |
There was a problem hiding this comment.
maybe cast is better here?
There was a problem hiding this comment.
Since there is
public function configureSettings(OptionsResolver $resolver): void
{
$resolver->setDefaults([
'limit' => 10,
'template' => '@SonataDoctrineORMAdmin/Block/block_audit.html.twig',
]);
}
What do you think about using allowedTypes ?
There was a problem hiding this comment.
Is that a feature from the options resolver? I think it would be great but not sure if phpstan will understand it tho.
There was a problem hiding this comment.
Phpstan won't understand, so I'll keep the assert, but it still a code improvement
|
|
||
| public function execute(BlockContextInterface $blockContext, ?Response $response = null): Response | ||
| { | ||
| $template = $blockContext->getTemplate(); |
There was a problem hiding this comment.
can a block return null here?
There was a problem hiding this comment.
I assume that no because of
public function configureSettings(OptionsResolver $resolver): void
{
$resolver->setDefaults([
'limit' => 10,
'template' => '@SonataDoctrineORMAdmin/Block/block_audit.html.twig',
]);
}
| $this->getQuery()->setFirstResult(null); | ||
| $this->getQuery()->setMaxResults(null); | ||
| $query = $this->getQuery(); | ||
| if (null === $query) { |
There was a problem hiding this comment.
Probably out of scope of this Pr, but wouldnt be nice if we ensure somehow that the query is never null, maybe as a parameter of this method or as a parameter of construct?
There was a problem hiding this comment.
Currently the pager and the query are constructor param of the Datagrid.
In this one we're running
$this->pager->setQuery($this->query);
$this->pager->init();
Maybe we could remove setQuery and use the query as a param of init indeed.
It's more a SonataAdmin change, so I'll recommend an issue.
| $loader->load('doctrine_orm_filter_types.xml'); | ||
|
|
||
| $bundles = $container->getParameter('kernel.bundles'); | ||
| \assert(\is_array($bundles)); |
There was a problem hiding this comment.
seems strange having to add this around each getParameter to ensure type
There was a problem hiding this comment.
getParameter is considered as returning mixed for phpstan because of the symfony phpdoc
| public function testDelete(): void | ||
| { | ||
| $entityManager = static::bootKernel()->getContainer()->get('doctrine')->getManager(); | ||
| $doctrine = static::bootKernel()->getContainer()->get('doctrine'); |
There was a problem hiding this comment.
I think the container resolution can be solved warming up cache before running phpstan. (can be done executing test previously). Then you add the xml to the phpstan config and it can solve those issues
|
This is ready @sonata-project/contributors :) |
4 participants
Need