Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise phpstan to max level #1386

Merged
merged 3 commits into from
Apr 11, 2021
Merged

Raise phpstan to max level #1386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cb57898
Raise phpstan to max level
VincentLanglet Apr 1, 2021
0ef688b
Bump doctrine
VincentLanglet Apr 9, 2021
5ed40e6
Update type phpdoc
VincentLanglet Apr 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
],
"require": {
"php": "^7.3 || ^8.0",
"doctrine/doctrine-bundle": "^1.8 || ^2.0",
"doctrine/doctrine-bundle": "^1.8 || ^2.3",
"doctrine/orm": "^2.5",
"doctrine/persistence": "^1.3.4 || ^2.0",
"sonata-project/admin-bundle": "4.x-dev",
Expand Down
3 changes: 3 additions & 0 deletions phpstan-baseline.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ parameters:
- # https://github.com/phpstan/phpstan/issues/4650 & https://github.com/phpstan/phpstan-src/pull/476
message: '#^Strict comparison using === between array\(\) and array<int\|string>\&nonEmpty will always evaluate to false.$#'
path: src/Model/ModelManager.php
- # https://github.com/doctrine/persistence/pull/163
message: '#Parameter \#1 \$class of method Sonata\\DoctrineORMAdminBundle\\FieldDescription\\FieldDescriptionFactory\:\:getMetadata\(\) expects class-string, string given.$#'
path: src/FieldDescription/FieldDescriptionFactory.php
2 changes: 1 addition & 1 deletion phpstan.neon.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ includes:
- phpstan-baseline.neon

parameters:
level: 6
level: max
bootstrapFiles:
- vendor/bin/.phpunit/phpunit/vendor/autoload.php
paths:
Expand Down
9 changes: 7 additions & 2 deletions src/Block/AuditBlockService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,21 @@ public function __construct(Environment $twig, AuditReader $auditReader)

public function execute(BlockContextInterface $blockContext, ?Response $response = null): Response
{
$template = $blockContext->getTemplate();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can a block return null here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume that no because of

public function configureSettings(OptionsResolver $resolver): void
    {
        $resolver->setDefaults([
            'limit' => 10,
            'template' => '@SonataDoctrineORMAdmin/Block/block_audit.html.twig',
        ]);
    }

\assert(null !== $template);
$limit = $blockContext->getSetting('limit');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe cast is better here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that a feature from the options resolver? I think it would be great but not sure if phpstan will understand it tho.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phpstan won't understand, so I'll keep the assert, but it still a code improvement

\assert(\is_int($limit));

$revisions = [];

foreach ($this->auditReader->findRevisionHistory($blockContext->getSetting('limit'), 0) as $revision) {
foreach ($this->auditReader->findRevisionHistory($limit, 0) as $revision) {
$revisions[] = [
'revision' => $revision,
'entities' => $this->auditReader->findEntitiesChangedAtRevision($revision->getRev()),
];
}

return $this->renderResponse($blockContext->getTemplate(), [
return $this->renderResponse($template, [
'block' => $blockContext->getBlock(),
'settings' => $blockContext->getSettings(),
'revisions' => $revisions,
Expand Down
6 changes: 6 additions & 0 deletions src/Builder/DatagridBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@ public function addFilter(DatagridInterface $datagrid, ?string $type, FieldDescr
{
if (null === $type) {
$guessType = $this->guesser->guess($fieldDescription);
if (null === $guessType) {
throw new \InvalidArgumentException(sprintf(
'Cannot guess a type for the field description "%s", You MUST provide a type.',
$fieldDescription->getName()
));
}

$type = $guessType->getType();
$fieldDescription->setType($type);
Expand Down
27 changes: 17 additions & 10 deletions src/Builder/ListBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ public function buildField(?string $type, FieldDescriptionInterface $fieldDescri
{
if (null === $type) {
$guessType = $this->guesser->guess($fieldDescription);
if (null === $guessType) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

throw new \InvalidArgumentException(sprintf(
'Cannot guess a type for the field description "%s", You MUST provide a type.',
$fieldDescription->getName()
));
}

$fieldDescription->setType($guessType->getType());
} else {
Expand All @@ -69,7 +75,16 @@ public function addField(FieldDescriptionCollection $list, ?string $type, FieldD

public function fixFieldDescription(FieldDescriptionInterface $fieldDescription): void
{
if (ListMapper::TYPE_ACTIONS === $fieldDescription->getType()) {
$type = $fieldDescription->getType();
if (!$type) {
throw new \RuntimeException(sprintf(
'Please define a type for field `%s` in `%s`',
$fieldDescription->getName(),
\get_class($fieldDescription->getAdmin())
));
}

if (ListMapper::TYPE_ACTIONS === $type) {
$this->buildActionFieldDescription($fieldDescription);
}

Expand All @@ -83,16 +98,8 @@ public function fixFieldDescription(FieldDescriptionInterface $fieldDescription)
$fieldDescription->setOption('_sort_order', $fieldDescription->getOption('_sort_order', 'ASC'));
}

if (!$fieldDescription->getType()) {
throw new \RuntimeException(sprintf(
'Please define a type for field `%s` in `%s`',
$fieldDescription->getName(),
\get_class($fieldDescription->getAdmin())
));
}

if (!$fieldDescription->getTemplate()) {
$fieldDescription->setTemplate($this->getTemplate($fieldDescription->getType()));
$fieldDescription->setTemplate($this->getTemplate($type));

if (!$fieldDescription->getTemplate()) {
switch ($fieldDescription->getMappingType()) {
Expand Down
12 changes: 10 additions & 2 deletions src/Builder/ShowBuilder.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ public function addField(FieldDescriptionCollection $list, ?string $type, FieldD
{
if (null === $type) {
$guessType = $this->guesser->guess($fieldDescription);
if (null === $guessType) {
throw new \InvalidArgumentException(sprintf(
'Cannot guess a type for the field description "%s", You MUST provide a type.',
$fieldDescription->getName()
));
}

$fieldDescription->setType($guessType->getType());
} else {
$fieldDescription->setType($type);
Expand All @@ -62,7 +69,8 @@ public function addField(FieldDescriptionCollection $list, ?string $type, FieldD

public function fixFieldDescription(FieldDescriptionInterface $fieldDescription): void
{
if (!$fieldDescription->getType()) {
$type = $fieldDescription->getType();
if (!$type) {
throw new \RuntimeException(sprintf(
'Please define a type for field `%s` in `%s`',
$fieldDescription->getName(),
Expand All @@ -71,7 +79,7 @@ public function fixFieldDescription(FieldDescriptionInterface $fieldDescription)
}

if (!$fieldDescription->getTemplate()) {
$fieldDescription->setTemplate($this->getTemplate($fieldDescription->getType()));
$fieldDescription->setTemplate($this->getTemplate($type));
}

switch ($fieldDescription->getMappingType()) {
Expand Down
31 changes: 13 additions & 18 deletions src/Datagrid/Pager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,9 @@ public function getCurrentPageResults(): iterable
->getMetadataFor(current($query->getQueryBuilder()->getRootEntities()))
->getIdentifierFieldNames();

// NEXT_MAJOR: Remove the check and the else part.
if (method_exists($query, 'getDoctrineQuery')) {
// Paginator with fetchJoinCollection doesn't work with composite primary keys
// https://github.com/doctrine/orm/issues/2910
$paginator = new Paginator($query->getDoctrineQuery(), 1 === \count($identifierFieldNames));
} else {
$paginator = new Paginator($query->getQueryBuilder(), 1 === \count($identifierFieldNames));
}
// Paginator with fetchJoinCollection doesn't work with composite primary keys
// https://github.com/doctrine/orm/issues/2910
$paginator = new Paginator($query->getDoctrineQuery(), 1 === \count($identifierFieldNames));

return $paginator->getIterator();
}
Expand All @@ -68,8 +63,13 @@ public function init(): void
{
$this->resultsCount = $this->computeResultsCount();

$this->getQuery()->setFirstResult(null);
$this->getQuery()->setMaxResults(null);
$query = $this->getQuery();
if (null === $query) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@VincentLanglet VincentLanglet Apr 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw new \LogicException('The pager need a query to be initialised');
}

$query->setFirstResult(null);
$query->setMaxResults(null);

if (0 === $this->getPage() || 0 === $this->getMaxPerPage() || 0 === $this->countResults()) {
$this->setLastPage(0);
Expand All @@ -78,8 +78,8 @@ public function init(): void

$this->setLastPage((int) ceil($this->countResults() / $this->getMaxPerPage()));

$this->getQuery()->setFirstResult($offset);
$this->getQuery()->setMaxResults($this->getMaxPerPage());
$query->setFirstResult($offset);
$query->setMaxResults($this->getMaxPerPage());
}
}

Expand All @@ -91,12 +91,7 @@ private function computeResultsCount(): int
throw new \TypeError(sprintf('The pager query MUST implement %s.', ProxyQueryInterface::class));
}

// NEXT_MAJOR: Remove the check and the else part.
if (method_exists($query, 'getDoctrineQuery')) {
$paginator = new Paginator($query->getDoctrineQuery());
} else {
$paginator = new Paginator($query->getQueryBuilder());
}
$paginator = new Paginator($query->getDoctrineQuery());

return \count($paginator);
}
Expand Down
4 changes: 2 additions & 2 deletions src/Datagrid/ProxyQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,11 +163,11 @@ public function getDoctrineQuery(): Query

$rootAlias = current($queryBuilder->getRootAliases());

if ($this->getSortBy()) {
$sortBy = $this->getSortBy();
if (null !== $sortBy) {
$orderByDQLPart = $queryBuilder->getDQLPart('orderBy');
$queryBuilder->resetDQLPart('orderBy');

$sortBy = $this->getSortBy();
if (false === strpos($sortBy, '.')) {
$sortBy = $rootAlias.'.'.$sortBy;
}
Expand Down
7 changes: 6 additions & 1 deletion src/DependencyInjection/Compiler/AddAuditEntityCompilerPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,12 @@ public function process(ContainerBuilder $container): void
private function getModelName(ContainerBuilder $container, string $name): string
{
if ('%' === $name[0]) {
return $container->getParameter(substr($name, 1, -1));
$parameter = $container->getParameter(substr($name, 1, -1));
if (!\is_string($parameter)) {
throw new \InvalidArgumentException(sprintf('Cannot find the model name "%s"', $name));
}

return $parameter;
}

return $name;
Expand Down
2 changes: 2 additions & 0 deletions src/DependencyInjection/Compiler/AddTemplatesCompilerPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ final class AddTemplatesCompilerPass implements CompilerPassInterface
public function process(ContainerBuilder $container): void
{
$overwrite = $container->getParameter('sonata.admin.configuration.admin_services');
\assert(\is_array($overwrite));
$templates = $container->getParameter('sonata_doctrine_orm_admin.templates');
\assert(\is_array($templates));

foreach ($container->findTaggedServiceIds('sonata.admin') as $id => $attributes) {
if (!isset($attributes[0]['manager_type']) || 'orm' !== $attributes[0]['manager_type']) {
Expand Down
1 change: 1 addition & 0 deletions src/DependencyInjection/SonataDoctrineORMAdminExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public function load(array $configs, ContainerBuilder $container): void
$loader->load('doctrine_orm_filter_types.xml');

$bundles = $container->getParameter('kernel.bundles');
\assert(\is_array($bundles));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems strange having to add this around each getParameter to ensure type

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getParameter is considered as returning mixed for phpstan because of the symfony phpdoc


if (isset($bundles['SimpleThingsEntityAuditBundle'])) {
$loader->load('audit.xml');
Expand Down
2 changes: 2 additions & 0 deletions src/FieldDescription/FieldDescriptionFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ public function create(string $class, string $name, array $options = []): FieldD
}

/**
* @phpstan-param class-string $baseClass
*
* @phpstan-return array{ClassMetadata, string, mixed[]}
*/
private function getParentMetadataForProperty(string $baseClass, string $propertyFullName): array
Expand Down
8 changes: 4 additions & 4 deletions src/Filter/ChoiceFilter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ public function getRenderSettings(): array
/**
* @param mixed[] $data
*
* @phpstan-param array{type?: string|int|null, value?: mixed} $data
* @phpstan-param array{type: int|null, value: mixed} $data
*/
private function filterWithMultipleValues(ProxyQueryInterface $query, string $alias, string $field, array $data = []): void
private function filterWithMultipleValues(ProxyQueryInterface $query, string $alias, string $field, array $data): void
{
if (0 === \count($data['value'])) {
return;
Expand Down Expand Up @@ -88,9 +88,9 @@ private function filterWithMultipleValues(ProxyQueryInterface $query, string $al
/**
* @param mixed[] $data
*
* @phpstan-param array{type?: string|int|null, value?: mixed} $data
* @phpstan-param array{type: int|null, value: mixed} $data
*/
private function filterWithSingleValue(ProxyQueryInterface $query, string $alias, string $field, array $data = []): void
private function filterWithSingleValue(ProxyQueryInterface $query, string $alias, string $field, array $data): void
{
if ('' === $data['value'] || false === $data['value']) {
return;
Expand Down
4 changes: 2 additions & 2 deletions src/Filter/Filter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ abstract class Filter extends BaseFilter
*
* @param mixed[] $data
*
* @phpstan-param array{type?: string|int|null, value?: mixed} $data
* @phpstan-param array{type?: int|null, value?: mixed} $data
*/
abstract public function filter(ProxyQueryInterface $query, string $alias, string $field, array $data): void;

Expand All @@ -65,7 +65,7 @@ public function isActive(): bool
*
* @return string[]
*
* @phpstan-param array{type?: string|int|null, value?: mixed} $data
* @phpstan-param array{type?: int|null, value?: mixed} $data
*/
protected function association(ProxyQueryInterface $query, array $data): array
{
Expand Down
16 changes: 6 additions & 10 deletions src/Model/ModelManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public function getLockVersion(object $object)
{
$metadata = $this->getMetadata(ClassUtils::getClass($object));

if (!$metadata->isVersioned) {
if (!$metadata->isVersioned || !isset($metadata->reflFields[$metadata->versionField])) {
return null;
}

Expand Down Expand Up @@ -243,17 +243,16 @@ public function getIdentifierValues(object $entity): array
}

$fieldType = $metadata->getTypeOfField($name);
$type = $fieldType && Type::hasType($fieldType) ? Type::getType($fieldType) : null;
if ($type) {
$identifiers[] = $this->getValueFromType($value, $type, $fieldType, $platform);
if (null !== $fieldType && Type::hasType($fieldType)) {
$identifiers[] = $this->getValueFromType($value, Type::getType($fieldType), $fieldType, $platform);

continue;
}

$identifierMetadata = $this->getMetadata(ClassUtils::getClass($value));

foreach ($identifierMetadata->getIdentifierValues($value) as $value) {
$identifiers[] = $value;
foreach ($identifierMetadata->getIdentifierValues($value) as $identifierValue) {
$identifiers[] = $identifierValue;
}
}

Expand Down Expand Up @@ -396,10 +395,7 @@ private function getFieldName(ClassMetadata $metadata, string $name): string
return $name;
}

/**
* @param mixed $value
*/
private function getValueFromType($value, Type $type, string $fieldType, AbstractPlatform $platform): string
private function getValueFromType(object $value, Type $type, string $fieldType, AbstractPlatform $platform): string
{
if ($platform->hasDoctrineTypeMappingFor($fieldType) &&
'binary' === $platform->getDoctrineTypeMapping($fieldType)
Expand Down
22 changes: 16 additions & 6 deletions tests/App/DataFixtures/BookFixtures.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@
use Doctrine\Bundle\FixturesBundle\Fixture;
use Doctrine\Common\DataFixtures\DependentFixtureInterface;
use Doctrine\Persistence\ObjectManager;
use Sonata\DoctrineORMAdminBundle\Tests\App\Entity\Author;
use Sonata\DoctrineORMAdminBundle\Tests\App\Entity\Book;
use Sonata\DoctrineORMAdminBundle\Tests\App\Entity\Category;
use Sonata\DoctrineORMAdminBundle\Tests\App\Entity\Reader;

final class BookFixtures extends Fixture implements DependentFixtureInterface
Expand All @@ -25,18 +27,26 @@ final class BookFixtures extends Fixture implements DependentFixtureInterface

public function load(ObjectManager $manager): void
{
$book = new Book('book_id', 'Don Quixote', $this->getReference(AuthorFixtures::AUTHOR));
$book->addCategory($this->getReference(CategoryFixtures::CATEGORY));
$author = $this->getReference(AuthorFixtures::AUTHOR);
\assert($author instanceof Author);
$category = $this->getReference(CategoryFixtures::CATEGORY);
\assert($category instanceof Category);

$book = new Book('book_id', 'Don Quixote', $author);
$book->addCategory($category);

$manager->persist($book);

$book1 = new Book('book_1', 'Book 1', $this->getReference(AuthorFixtures::AUTHOR_WITH_TWO_BOOKS));
$book1->addCategory($this->getReference(CategoryFixtures::CATEGORY));
$authorWithTwoBooks = $this->getReference(AuthorFixtures::AUTHOR_WITH_TWO_BOOKS);
\assert($authorWithTwoBooks instanceof Author);

$book1 = new Book('book_1', 'Book 1', $authorWithTwoBooks);
$book1->addCategory($category);

$this->addReaders($book1, 100);

$book2 = new Book('book_2', 'Book 2', $this->getReference(AuthorFixtures::AUTHOR_WITH_TWO_BOOKS));
$book2->addCategory($this->getReference(CategoryFixtures::CATEGORY));
$book2 = new Book('book_2', 'Book 2', $authorWithTwoBooks);
$book2->addCategory($category);

$this->addReaders($book2, 100);

Expand Down
Loading