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.

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

Unlock phpstan level 5 #1924

Merged
merged 21 commits into from
Jul 17, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ jobs:

- name: Analyze the code
run: |
vendor/bin/phpstan analyse core-bundle/src core-bundle/tests --level=4 --no-progress
vendor/bin/phpstan analyse core-bundle/src core-bundle/tests --level=5 --no-progress
vendor/bin/psalm --no-suggestions --threads=4 --no-progress

- name: Analyze the YAML files
Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@
"vendor/bin/monorepo-tools composer-json --validate --ansi"
],
"phpstan": [
"vendor/bin/phpstan analyze core-bundle/src core-bundle/tests --level=4 --memory-limit=1G --ansi"
"vendor/bin/phpstan analyze core-bundle/src core-bundle/tests --level=5 --memory-limit=1G --ansi"
],
"psalm": [
"vendor/bin/psalm --no-suggestions --threads=4"
Expand Down
2 changes: 1 addition & 1 deletion core-bundle/src/Composer/ScriptHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ static function (string $type, string $buffer) use ($event): void {
);

if (!$process->isSuccessful()) {
throw new \RuntimeException(sprintf('An error occurred while executing the "%s" command.', $cmd));
throw new \RuntimeException(sprintf('An error occurred while executing the "%s" command.', implode(' ', $cmd)));
Copy link
Member Author

Choose a reason for hiding this comment

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

(This was introduced with #1917 and therefore must not be backported.)

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ private function getFragmentTags(ContainerBuilder $container, string $tag): arra
$result = [];

foreach ($this->findAndSortTaggedServices($tag, $container) as $reference) {
$definition = $container->findDefinition($reference);
$definition = $container->findDefinition((string) $reference);

foreach ($definition->getTag($tag) as $attributes) {
if (!isset($attributes['category'])) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ protected function registerFragments(ContainerBuilder $container, string $tag):
$command = $container->findDefinition('contao.command.debug_fragments');

foreach ($this->findAndSortTaggedServices($tag, $container) as $reference) {
$definition = $container->findDefinition($reference);
$definition = $container->findDefinition((string) $reference);
$definition->setPublic(true);

$tags = $definition->getTag($tag);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public function __invoke(ResponseEvent $event): void
$response = $event->getResponse();

// Disable the default Symfony auto cache control
$response->headers->set(AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER, true);
$response->headers->set(AbstractSessionListener::NO_AUTO_CACHE_CONTROL_HEADER, '1');
leofeyer marked this conversation as resolved.
Show resolved Hide resolved

// If the response is not cacheable for a reverse proxy, we don't have to do anything anyway
if (!$response->isCacheable()) {
Expand Down
1 change: 1 addition & 0 deletions core-bundle/src/Image/ImageFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ public function setPredefinedSizes(array $predefinedSizes): void

public function create($path, $size = null, $options = null): ImageInterface
{
/** @var mixed $options */
leofeyer marked this conversation as resolved.
Show resolved Hide resolved
if (null !== $options && !\is_string($options) && !$options instanceof ResizeOptions) {
throw new \InvalidArgumentException('Options must be of type null, string or '.ResizeOptions::class);
}
Expand Down
4 changes: 2 additions & 2 deletions core-bundle/src/Resources/contao/library/Contao/Config.php
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ public static function has($strKey)
*
* @param string $strKey The short key
*
* @return mixed|null The configuration value
* @return mixed The configuration value
*/
public static function get($strKey)
{
Expand All @@ -374,7 +374,7 @@ public static function get($strKey)
* Temporarily set a configuration value
*
* @param string $strKey The short key
* @param string $varValue The configuration value
* @param mixed $varValue The configuration value
*/
public static function set($strKey, $varValue)
{
Expand Down
4 changes: 2 additions & 2 deletions core-bundle/src/Resources/contao/library/Contao/Picture.php
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class Picture
/**
* The image size items collection
*
* @var ImageSizeItemModel[]|Collection
* @var array<ImageSizeItemModel|object>|Collection
leofeyer marked this conversation as resolved.
Show resolved Hide resolved
*/
protected $imageSizeItems = array();

Expand Down Expand Up @@ -194,7 +194,7 @@ public function setImageSize($imageSize)
/**
* Set the image size items collection
*
* @param ImageSizeItemModel[]|Collection $imageSizeItems The image size items collection
* @param array<ImageSizeItemModel|object>|Collection $imageSizeItems The image size items collection
*
* @return $this The picture object
*/
Expand Down
6 changes: 3 additions & 3 deletions core-bundle/src/Resources/contao/library/Contao/System.php
Original file line number Diff line number Diff line change
Expand Up @@ -367,9 +367,9 @@ public static function getReferer($blnEncodeAmpersands=false, $strTable=null)
/**
* Load a set of language files
*
* @param string $strName The table name
* @param boolean $strLanguage An optional language code
* @param boolean $blnNoCache If true, the cache will be bypassed
* @param string $strName The table name
* @param string|null $strLanguage An optional language code
* @param boolean $blnNoCache If true, the cache will be bypassed
*/
public static function loadLanguageFile($strName, $strLanguage=null, $blnNoCache=false)
{
Expand Down
8 changes: 5 additions & 3 deletions core-bundle/src/Routing/RouteProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
use Contao\CoreBundle\ContaoCoreBundle;
use Contao\CoreBundle\Exception\NoRootPageFoundException;
use Contao\CoreBundle\Framework\ContaoFramework;
use Contao\Model;
use Contao\Model\Collection;
use Contao\PageModel;
use Contao\System;
Expand Down Expand Up @@ -458,7 +457,7 @@ static function (Route $a, Route $b) use ($languages, $routes) {
}

/**
* @return array<Model>
* @return array<PageModel>
*/
private function findPages(array $candidates): array
{
Expand Down Expand Up @@ -491,11 +490,12 @@ private function findPages(array $candidates): array
return [];
}

/** @var array<PageModel> */
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we add @mixin<T> and @return array<T> (or some other generics magic) to the Model class so PhpStan knows what we are returning 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.

That's a tough one. We would imo need to add generics to the Collection, the Adapter and the Model for this to work. And then we will end with a lot of @phpstan-param, @phpstan-return, @phpstan-var... declarations so that our IDEs are still able to guess the type.

Everything would look a bit like this I guess: https://phpstan.org/r/15377a79-1be1-4f2d-999f-f1ae9dbe37f0

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to try?

Copy link
Member Author

Choose a reason for hiding this comment

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

Challenge accepted. Are you ok with all those tags?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I am. We are already using them in the Adapter class anyway.

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 had a deep dive into the topic.

Most of the things could theoretically be solved by having a Collection<T of Model> + static/Collection<static> return type annotations in the model base class and forced return overwrites for all of the findBy* methods because of the dynamic Model vs Collection vs Array return type nature of find().

Unfortunately, in reality this doesn't work as expected because we're unfortunately using __callStatic and PHPStan then is not able to resolve the types.

It would possibly work to create our own extension, but well...

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to modify https://github.com/contao/phpstan 😎

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to modify https://github.com/contao/phpstan sunglasses

If I only knew how. 😄

Copy link
Member

Choose a reason for hiding this comment

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

I have created a follow-up issue here: #1940

return $pages->getModels();
}

/**
* @return array<Model>
* @return array<PageModel>
*/
private function findRootPages(string $httpHost): array
{
Expand Down Expand Up @@ -523,12 +523,14 @@ private function findRootPages(string $httpHost): array
$pages = $pageModel->findBy(["(tl_page.type='root' AND (tl_page.dns=? OR tl_page.dns=''))"], $httpHost);

if ($pages instanceof Collection) {
/** @var array<PageModel> $rootPages */
$rootPages = $pages->getModels();
}

$pages = $pageModel->findBy(["tl_page.alias='index' OR tl_page.alias='/'"], null);

if ($pages instanceof Collection) {
/** @var array<PageModel> $indexPages */
$indexPages = $pages->getModels();
}

Expand Down
12 changes: 9 additions & 3 deletions core-bundle/src/Routing/UrlGenerator.php
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,10 @@ private function prepareDomain(RequestContext $context, array &$parameters, int
*/
private function addHostToContext(RequestContext $context, array $parameters, int &$referenceType): void
{
/**
* @var string $host
* @var int|null $port
*/
[$host, $port] = $this->getHostAndPort($parameters['_domain']);

if ($context->getHost() === $host) {
Expand All @@ -173,7 +177,7 @@ private function addHostToContext(RequestContext $context, array $parameters, in
$context->setHost($host);
$referenceType = UrlGeneratorInterface::ABSOLUTE_URL;

if (!$port) {
if (null === $port) {
leofeyer marked this conversation as resolved.
Show resolved Hide resolved
return;
}

Expand All @@ -187,12 +191,14 @@ private function addHostToContext(RequestContext $context, array $parameters, in
/**
* Extracts host and port from the domain.
*
* @return array<(string|null)>
leofeyer marked this conversation as resolved.
Show resolved Hide resolved
* @return array<string|int|null>
*/
private function getHostAndPort(string $domain): array
{
if (false !== strpos($domain, ':')) {
return explode(':', $domain, 2);
[$host, $port] = explode(':', $domain, 2);

return [$host, (int) $port];
}

return [$domain, null];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@ public function testMergesCacheControlHeader(): void
$this->onKernelRequest($subscriber, KernelInterface::MASTER_REQUEST);

$subResponse = new Response();
$subResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, true);
$subResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, '1');
leofeyer marked this conversation as resolved.
Show resolved Hide resolved
$subResponse->setPublic();
$subResponse->setMaxAge(30);

$this->onKernelResponse($subscriber, $subResponse, KernelInterface::SUB_REQUEST);

$mainResponse = new Response();
$mainResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, true);
$mainResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, '1');
$mainResponse->setPublic();
$mainResponse->setMaxAge(60);

Expand All @@ -66,13 +66,13 @@ public function testMakeMasterResponsePrivateIfSubrequestIsPrivate(): void
$this->onKernelRequest($subscriber, KernelInterface::MASTER_REQUEST);

$subResponse = new Response();
$subResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, true);
$subResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, '1');
$subResponse->setPrivate();

$this->onKernelResponse($subscriber, $subResponse, KernelInterface::SUB_REQUEST);

$mainResponse = new Response();
$mainResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, true);
$mainResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, '1');
$subResponse->setPublic();
$mainResponse->setMaxAge(60);

Expand All @@ -93,7 +93,7 @@ public function testIgnoresSubrequestWithoutMergeHeader(): void
$this->onKernelResponse($subscriber, $subResponse, KernelInterface::SUB_REQUEST);

$mainResponse = new Response();
$mainResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, true);
$mainResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, '1');
$mainResponse->setPublic();
$mainResponse->setMaxAge(60);

Expand All @@ -109,13 +109,13 @@ public function testIgnoresSubrequestWithoutCacheControlHeader(): void
$this->onKernelRequest($subscriber, KernelInterface::MASTER_REQUEST);

$subResponse = new Response();
$subResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, true);
$subResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, '1');
$subResponse->headers->remove('Cache-Control');

$this->onKernelResponse($subscriber, $subResponse, KernelInterface::SUB_REQUEST);

$mainResponse = new Response();
$mainResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, true);
$mainResponse->headers->set(SubrequestCacheSubscriber::MERGE_CACHE_HEADER, '1');
$mainResponse->setPublic();
$mainResponse->setMaxAge(60);

Expand Down
6 changes: 3 additions & 3 deletions core-bundle/tests/OptIn/OptInTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public function testCreatesAToken(): void
$model
->expects($this->once())
->method('setRelatedRecords')
->with(['tl_member' => 1])
->with(['tl_member' => [1]])
;

$framework = $this->mockContaoFramework();
Expand All @@ -44,7 +44,7 @@ public function testCreatesAToken(): void
->willReturn($model)
;

$token = (new OptIn($framework))->create('reg', 'foo@bar.com', ['tl_member' => 1]);
$token = (new OptIn($framework))->create('reg', 'foo@bar.com', ['tl_member' => [1]]);

$this->assertStringMatchesFormat('reg-%x', $token->getIdentifier());
$this->assertTrue($token->isValid());
Expand All @@ -63,7 +63,7 @@ public function testDoesNotCreateATokenIfThePrefixIsTooLong(): void
$this->expectException('InvalidArgumentException');
$this->expectExceptionMessage('The token prefix must not be longer than 6 characters');

(new OptIn($framework))->create('registration', 'foo@bar.com', ['tl_member' => 1]);
(new OptIn($framework))->create('registration', 'foo@bar.com', ['tl_member' => [1]]);
}

public function testFindsAToken(): void
Expand Down
19 changes: 12 additions & 7 deletions phpstan.neon.dist
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ parameters:
symfony:
container_xml_path: %currentWorkingDirectory%/core-bundle/var/cache/phpstan/appContao_CoreBundle_Tests_Functional_app_AppKernelPhpstanDebugContainer.xml

dynamicConstantNames:
- BE_USER_LOGGED_IN

excludes_analyse:
- %currentWorkingDirectory%/core-bundle/src/Resources/*
- %currentWorkingDirectory%/core-bundle/tests/Fixtures/*
Expand All @@ -43,13 +46,16 @@ parameters:
- %currentWorkingDirectory%/core-bundle/tests/Framework/ContaoFrameworkTest.php
- %currentWorkingDirectory%/core-bundle/tests/Doctrine/Schema/DcaSchemaProviderTest.php

- message: '#Result of && is always false\.#'
# Ignore intentionally invalid User class in ContaoUserProviderTest.
- message: '#Parameter \#1 .* of method Contao\\CoreBundle\\Security\\User\\ContaoUserProvider::upgradePassword\(\) expects Contao\\User, .*&Symfony\\Component\\Security\\Core\\User\\UserInterface given\.#'
paths:
- %currentWorkingDirectory%/core-bundle/src/DataCollector/ContaoDataCollector.php
- %currentWorkingDirectory%/core-bundle/src/Image/ImageFactory.php
- %currentWorkingDirectory%/core-bundle/tests/Security/User/ContaoUserProviderTest.php

- message: '#Strict comparison using === between true and false will always evaluate to false\.#'
path: %currentWorkingDirectory%/core-bundle/src/DataCollector/ContaoDataCollector.php
# Ignore tests intentionally calling legacy code with other types than annotated
- message: '#Parameter .* of (static )?method .* expects .* given.#'
paths:
- %currentWorkingDirectory%/core-bundle/tests/Contao/ImageTest.php
- %currentWorkingDirectory%/core-bundle/tests/Routing/UrlGeneratorTest.php

# Ignore that $container->get() always returns false (see https://github.com/phpstan/phpstan-symfony/issues/74)
- message: '#Negated boolean expression is always (false|true)\.#'
Expand All @@ -61,8 +67,7 @@ parameters:
- message: '#Unreachable statement - code above always terminates\.#'
paths:
- %currentWorkingDirectory%/core-bundle/src/Controller/AbstractController.php
- %currentWorkingDirectory%/core-bundle/src/Command/MigrateCommand.php

reportUnmatchedIgnoredErrors: false
reportUnmatchedIgnoredErrors: true
checkGenericClassInNonGenericObjectType: false
inferPrivatePropertyTypeFromConstructor: true