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

[CI] Added phpstan #60

Merged
merged 4 commits into from
Dec 23, 2024
Merged

[CI] Added phpstan #60

merged 4 commits into from
Dec 23, 2024

Conversation

ViniTou
Copy link
Contributor

@ViniTou ViniTou commented Dec 23, 2024

🎫 Issue IBX-XXXXX

Description:

For QA:

Documentation:

@ViniTou ViniTou force-pushed the add-phpstan branch 2 times, most recently from 7ec0987 to 0653c34 Compare December 23, 2024 11:45
@ViniTou ViniTou requested a review from a team December 23, 2024 11:57
@@ -272,6 +272,9 @@ private static function getStability(ComposerSystemInfo $composerInfo): string
return Stability::STABILITIES[$stabilityFlag];
}

/**
* @param list<string> $packageNames
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beacuse I do not know if it is int or string indexed, and it dosent matter for this function. List is just less hustle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually...

list<X> is basically equal to array<int, X>, with added assumption that keys will be always incrementing by 1. So it is in fact more narrow than array.

But yes, it should not matter in this context.

@@ -146,7 +146,7 @@ class IbexaSystemInfoCollector implements SystemInfoCollector
private $kernelProjectDir;

/**
* @param \Ibexa\Bundle\SystemInfo\SystemInfo\Collector\JsonComposerLockSystemInfoCollector|\Ibexa\Bundle\SystemInfo\SystemInfo\Collector\SystemInfoCollector $composerCollector
* @param \Ibexa\Bundle\SystemInfo\SystemInfo\Collector\JsonComposerLockSystemInfoCollector|SystemInfoCollector $composerCollector
Copy link
Contributor

Choose a reason for hiding this comment

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

FQCN was removed by accident.

Suggested change
* @param \Ibexa\Bundle\SystemInfo\SystemInfo\Collector\JsonComposerLockSystemInfoCollector|SystemInfoCollector $composerCollector
* @param \Ibexa\Bundle\SystemInfo\SystemInfo\Collector\JsonComposerLockSystemInfoCollector|\Ibexa\Bundle\SystemInfo\SystemInfo\Collector\SystemInfoCollector $composerCollector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if that even make much sense to have \Ibexa\Bundle\SystemInfo\SystemInfo\Collector\JsonComposerLockSystemInfoCollector here, as it is implementation of \Ibexa\Bundle\SystemInfo\SystemInfo\Collector\SystemInfoCollector

Copy link
Contributor

Choose a reason for hiding this comment

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

If one is an interface that the other implements, then implementation can be removed from declaration.

tests/bundle/SystemInfo/Registry/IdentifierBasedTest.php Outdated Show resolved Hide resolved
use PHPUnit\Framework\TestCase;

class SystemInfoViewBuilderTest extends TestCase
{
private $configuratorMock;
private Configurator $configuratorMock;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be more specific to what actually is inside, and fix on of the still reported PHPStan issues:

Suggested change
private Configurator $configuratorMock;
/** @var \PHPUnit\Framework\MockObject\MockObject&\Ibexa\Core\MVC\Symfony\View\Configurator */
private Configurator $configuratorMock;

Similar thing can be done for registry and collector mocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I wanted to do proper union type in main. I can add this atm as well tho.

tests/bundle/View/SystemInfoViewBuilderTest.php Outdated Show resolved Hide resolved
@ViniTou ViniTou requested a review from Steveb-p December 23, 2024 12:53
@ViniTou ViniTou merged commit 67ece93 into 4.6 Dec 23, 2024
17 checks passed
@ViniTou ViniTou deleted the add-phpstan branch December 23, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants