Skip to content

feat: add tests folder with phpstan level 8 solved all errors #417

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

Open
shakaran wants to merge 11 commits into
base: master
Choose a base branch
from
Open

feat: add tests folder with phpstan level 8 solved all errors #417

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
73826fc
feat: solve phpstan error with null type declared
shakaran Dec 8, 2025
22f988d
feat: resolve phpstan errors in tests
shakaran Dec 9, 2025
65678ec
feat: update phpstan type for tests and errors
shakaran Dec 9, 2025
6e591d5
feat: resolve all errors in phpstan tests
shakaran Dec 9, 2025
f1db5be
fix: avoid assert not null changing MenuTestCase and ItemInterface
shakaran Dec 9, 2025
3f6fef6
fix: use IteratorIterator as suggested by @stof
shakaran Dec 9, 2025
2d5281f
fix: refactor tests for back to use ArrayAccess
shakaran Dec 9, 2025
b70d44a
fix: remove intermediate variables not needed anymore
shakaran Dec 9, 2025
584ec1e
fix: restore cases of ArrayAccess instead getChildren
shakaran Dec 9, 2025
2ccbffb
fix: remove intermediate variables and avoid getChildren()
shakaran Dec 9, 2025
cf50474
feat: add return types only nullables
shakaran Dec 9, 2025
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 tests/Knp/Menu/Tests/Iterator/CurrentItemFilterIteratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public function testSimpleFiltering(): void

$names = [];
// FilterIterator expects an Iterator implementation explicitly, not an IteratorAggregate.
$iterator = new CurrentItemFilterIterator($this->menu->getIterator(), new Matcher());
$iterator = new CurrentItemFilterIterator(new \ArrayIterator($this->menu->getChildren()), new Matcher());
Copy link
Copy Markdown
Collaborator

@stof stof Dec 9, 2025

Choose a reason for hiding this comment

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

I'd rather use new IteratorIterator($this->menu), which is the built-in way in PHP to wrap a Traversable into an Iterator.

Copy link
Copy Markdown
Contributor Author

@shakaran shakaran Dec 9, 2025

Choose a reason for hiding this comment

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

Fixed at 3f6fef6


foreach ($iterator as $value) {
$names[] = $value->getName();
Expand Down
2 changes: 1 addition & 1 deletion tests/Knp/Menu/Tests/Iterator/DisplayedItemFilterIteratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public function testFiltering(): void

$names = [];
$iterator = new \RecursiveIteratorIterator(
new DisplayedItemFilterIterator(new RecursiveItemIterator($this->menu)),
new DisplayedItemFilterIterator(new RecursiveItemIterator(new \ArrayIterator($this->menu->getChildren()))),
\RecursiveIteratorIterator::SELF_FIRST
);
foreach ($iterator as $value) {
Expand Down
2 changes: 1 addition & 1 deletion tests/Knp/Menu/Tests/Iterator/IteratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ final class IteratorTest extends MenuTestCase
public function testIterator(): void
{
$count = 0;
foreach ($this->pt1 as $key => $value) {
foreach ($this->pt1->getChildren() as $key => $value) {
Copy link
Copy Markdown
Collaborator

@stof stof Dec 9, 2025

Choose a reason for hiding this comment

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

why ? This should not be needed. This test is even meant to cover the iterable API of the menu IMO, based on its name.

Copy link
Copy Markdown
Contributor Author

@shakaran shakaran Dec 9, 2025

Choose a reason for hiding this comment

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

Fixed at 2ccbffb

++$count;
$this->assertEquals('Child '.$count, $key);
$this->assertEquals('Child '.$count, $value->getLabel());
Expand Down
21 changes: 18 additions & 3 deletions tests/Knp/Menu/Tests/Matcher/Voter/CallbackVoterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,23 @@ public function testMatching(callable $callback, ?bool $expected): void
*/
public static function provideData(): iterable
{
yield 'matching' => [fn (): ?bool => true, true];
yield 'not matching' => [fn (): ?bool => false, false];
yield 'skipping' => [fn (): ?bool => null, null];
yield 'matching' => [[self::class, 'matchingCallable'], true];
yield 'not matching' => [[self::class, 'notMatchingCallable'], false];
yield 'skipping' => [[self::class, 'skippingCallable'], null];
Copy link
Copy Markdown
Collaborator

@stof stof Dec 9, 2025

Choose a reason for hiding this comment

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

Why this change ?

Copy link
Copy Markdown
Contributor Author

@shakaran shakaran Dec 9, 2025

Choose a reason for hiding this comment

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

Originally was

Line CallbackVoterTest.php

:60 Anonymous function never returns null so it can be removed from the return type.
🪪 return.unusedType
:61 Anonymous function never returns null so it can be removed from the return type.
🪪 return.unusedType

But I over complicate doing a static function instead of closure. Reverting only solving the nullables at cf50474

}

private static function matchingCallable(): bool
{
return true;
}

private static function notMatchingCallable(): bool
{
return false;
}

private static function skippingCallable(): null
{
return null;
}
}
19 changes: 2 additions & 17 deletions tests/Knp/Menu/Tests/MenuFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,9 @@ public function testExtensions(): void
$factory = new MenuFactory();

$extension1 = $this->getMockBuilder(ExtensionInterface::class)->getMock();
$extension1->expects($this->once())
->method('buildOptions')
->with(['foo' => 'bar'])
->willReturn(['uri' => 'foobar']);
$extension1->expects($this->once())
->method('buildItem')
->with($this->isInstanceOf(ItemInterface::class), $this->containsCustom('foobar'));
->with($this->isInstanceOf(ItemInterface::class), $this->containsEqual('foobar'));

$factory->addExtension($extension1);

Expand All @@ -31,7 +27,7 @@ public function testExtensions(): void
->willReturn(['foo' => 'bar']);
$extension1->expects($this->once())
->method('buildItem')
->with($this->isInstanceOf(ItemInterface::class), $this->containsCustom('foobar'));
->with($this->isInstanceOf(ItemInterface::class), $this->containsEqual('foobar'));

$factory->addExtension($extension2, 10);

Expand All @@ -57,16 +53,5 @@ public function testCreateItem(): void
$this->assertEquals('foo', $item->getLinkAttribute('class'));
}

private function containsCustom(string $value): ?object
{
if (\method_exists($this, 'contains')) {
return $this->contains($value);
}

if (\method_exists($this, 'containsEqual')) {
return $this->containsEqual($value);
}

return null;
}
}
6 changes: 3 additions & 3 deletions tests/Knp/Menu/Tests/MenuItemGetterSetterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@ public function testChildren(): void
{
$menu = $this->createMenu();
$child = $this->createMenu('child_menu');
$menu->setChildren([$child]);
$this->assertEquals([$child], $menu->getChildren());
$menu->setChildren(['child' => $child]);
$this->assertEquals(['child' => $child], $menu->getChildren());
}

public function testSetExistingNameThrowsAnException(): void
Expand All @@ -182,7 +182,7 @@ public function testSetExistingNameThrowsAnException(): void
$menu = $this->createMenu();
$menu->addChild('jack');
$menu->addChild('joe');
$menu->getChild('joe')->setName('jack');
$menu->getChildren()['joe']->setName('jack');
}

public function testSetSameName(): void
Expand Down
30 changes: 15 additions & 15 deletions tests/Knp/Menu/Tests/MenuItemTreeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ final class MenuItemTreeTest extends MenuTestCase
{
public function testSampleTreeIntegrity(): void
{
$this->assertCount(2, $this->menu);
$this->assertCount(3, $this->menu['Parent 1']);
$this->assertCount(1, $this->menu['Parent 2']);
$this->assertCount(1, $this->menu['Parent 2']['Child 4']);
$this->assertEquals('Grandchild 1', $this->menu['Parent 2']['Child 4']['Grandchild 1']->getName());
$this->assertCount(2, $this->menu->getChildren());
$this->assertCount(3, $this->pt1->getChildren());
$this->assertCount(1, $this->pt2->getChildren());
$this->assertCount(1, $this->ch4->getChildren());
$this->assertEquals('Grandchild 1', $this->gc1->getName());
Copy link
Copy Markdown
Collaborator

@stof stof Dec 9, 2025

Choose a reason for hiding this comment

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

This is totally changing the assertions being done. It does not test the integrity of the ArrayAccess anymore.

Copy link
Copy Markdown
Contributor Author

@shakaran shakaran Dec 9, 2025

Choose a reason for hiding this comment

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

Fixed at 584ec1e and other cases for ensure the ArrayAccess instead getChildren

}

public function testGetLevel(): void
Expand Down Expand Up @@ -120,13 +120,13 @@ public function testActsLikeLastWithNoDisplayedItem(): void
public function testArrayAccess(): void
{
$this->menu->addChild('Child Menu');
$this->assertEquals('Child Menu', $this->menu['Child Menu']->getName());
$this->assertNull($this->menu['Fake']);
$this->assertEquals('Child Menu', $this->menu->getChildren()['Child Menu']->getName());
Copy link
Copy Markdown
Collaborator

@stof stof Dec 9, 2025

Choose a reason for hiding this comment

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

This is not testing the ArrayAccess implementation anymore, while this is what the test is about.

Copy link
Copy Markdown
Contributor Author

@shakaran shakaran Dec 9, 2025

Choose a reason for hiding this comment

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

Fixed at 2d5281f

$this->assertNull($this->menu->getChild('Fake'));

$this->menu['New Child'] = 'New Label';
$this->assertEquals(MenuItem::class, \get_class($this->menu['New Child']));
$this->assertEquals('New Child', $this->menu['New Child']->getName());
$this->assertEquals('New Label', $this->menu['New Child']->getLabel());
$this->menu->addChild('New Child', ['label' => 'New Label']);
$this->assertEquals(MenuItem::class, \get_class($this->menu->getChildren()['New Child']));
$this->assertEquals('New Child', $this->menu->getChildren()['New Child']->getName());
$this->assertEquals('New Label', $this->menu->getChildren()['New Child']->getLabel());

unset($this->menu['New Child']);
$this->assertNull($this->menu['New Child']);
Expand Down Expand Up @@ -204,14 +204,14 @@ public function testRemoveChild(): void
$this->assertCount(4, $this->ch4);
$this->ch4->removeChild('gc4');
$this->assertCount(3, $this->ch4);
$this->assertTrue($this->ch4->getChild('Grandchild 1')->isFirst());
$this->assertTrue($this->ch4->getChild('gc3')->isLast());
$this->assertTrue($this->gc1->isFirst());
$this->assertTrue($gc3->isLast());
}

public function testRemoveFakeChild(): void
{
$this->menu->removeChild('fake');
$this->assertCount(2, $this->menu);
$this->assertCount(2, $this->menu->getChildren());
}

public function testReAddRemovedChild(): void
Expand Down Expand Up @@ -243,7 +243,7 @@ public function testGetUri(): void
{
$this->addChildWithExternalUrl();
$this->assertNull($this->pt1->getUri());
$this->assertEquals('http://www.symfony-reloaded.org', $this->menu['child']->getUri());
$this->assertEquals('http://www.symfony-reloaded.org', $this->menu->getChildren()['child']->getUri());
}

protected function addChildWithExternalUrl(): void
Expand Down
16 changes: 8 additions & 8 deletions tests/Knp/Menu/Tests/MenuTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,21 +9,21 @@

abstract class MenuTestCase extends TestCase
{
protected ItemInterface|null $menu;
protected ?ItemInterface $menu;
Copy link
Copy Markdown
Collaborator

@stof stof Dec 9, 2025

Choose a reason for hiding this comment

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

To me, the right fix is to make all those properties non-nullable and removing the tearDown method that assigns them to null (which is useless as the Test object itself is dereferenced after it is tear down so everything will still be collected)

Copy link
Copy Markdown
Contributor Author

@shakaran shakaran Dec 9, 2025

Choose a reason for hiding this comment

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

Changed in f1db5be


protected ItemInterface|null $pt1;
protected ?ItemInterface $pt1;

protected ItemInterface|null $ch1;
protected ?ItemInterface $ch1;

protected ItemInterface|null $ch2;
protected ?ItemInterface $ch2;

protected ItemInterface|null $ch3;
protected ?ItemInterface $ch3;

protected ItemInterface|null $pt2;
protected ?ItemInterface $pt2;

protected ItemInterface|null $ch4;
protected ?ItemInterface $ch4;

protected ItemInterface|null $gc1;
protected ?ItemInterface $gc1;
Copy link
Copy Markdown
Contributor

@derrabus derrabus Dec 9, 2025

Choose a reason for hiding this comment

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

Why?

Copy link
Copy Markdown
Contributor Author

@shakaran shakaran Dec 9, 2025

Choose a reason for hiding this comment

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

It is just sugar syntax for union types, very common in my opinion in a lot projects

Copy link
Copy Markdown
Contributor

@derrabus derrabus Dec 9, 2025

Choose a reason for hiding this comment

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

Okay, so the change is abosutely unrealted to and irrelevant for the goal that you're trying to achieve? Let's revert it then?

Copy link
Copy Markdown
Contributor Author

@shakaran shakaran Dec 9, 2025

Choose a reason for hiding this comment

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

Changed in f1db5be


protected function setUp(): void
{
Expand Down
Loading