Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Fix build failures in develop #59

Merged
merged 3 commits into from Dec 31, 2019
Merged

Fix build failures in develop #59

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc42739
Fix build failures introduced with #56
tux-rampage Dec 16, 2019
e0536dc
Revert code change and add ignore
tux-rampage Dec 17, 2019
d75012d
Re-Add internal state assertions
tux-rampage Dec 17, 2019
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
9 changes: 6 additions & 3 deletions src/Definition/Reflection/Parameter.php
View file
Open in desktop
Expand Up @@ -63,11 +63,14 @@ public function getPosition() : int
*/
public function getType() : ?string
{
if ($this->reflection->hasType()) {
return $this->reflection->getType()->getName();
if (! $this->reflection->hasType()) {
return null;
}

return null;
$type = $this->reflection->getType();
assert($type !== null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PHPstan does not infer that the returned type is not null when hasType() is true. I guess asserting not-null is safe here.

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion this should be added then to the exceptions in phpstan configuration.

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'll try a more recent version of phpstan first. Adding ignores is also something I'd like to avoid when possible.

Maybe switching to psalm would also be an option. It feels a bit more reliable in comparison. At least it's type assertions and inference is quite powerful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upgrading PHPStan does not fix it and it will result in a bigger PR.

I reverted the code change and added an exclude to phpstan.neon, but this is awful. I'd like to rewrite this method like this:

    public function getType() : ?string
    {
        $type = $this->reflection->getType();
        return $type !== null ? $type->getName() : null;
    }

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

it has no sense

if we are checking first hasType() then getType() can't be null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess i was too slow. PHPstan does not infer it correctly. Not sure if an update will change this. Might try this as well-


return $type->getName();
}

/**
Expand Down
10 changes: 1 addition & 9 deletions test/CodeGenerator/AbstractInjectorTest.php
View file
Open in desktop
Expand Up @@ -187,17 +187,9 @@ public function testConstructionWithoutContainerUsesDefaultContainer()
public function testFactoryIsCreatedFromClassNameString()
{
$subject = $this->createTestSubject(function () {
return ['SomeClass' => StdClassFactory::class ];
return ['SomeClass' => StdClassFactory::class];
});

$this->assertSame(
StdClassFactory::class,
self::readAttribute($subject, 'factories')['SomeClass'] ?? null
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and the other assertion check the internal state of the class, which is considered bad practice and readAttribute() has been removed from phpunit (see sebastianbergmann/phpunit#3338).

Testing the result of create() should be sufficient.

$this->assertInstanceOf(stdClass::class, $subject->create('SomeClass'));
$this->assertInstanceOf(
StdClassFactory::class,
self::readAttribute($subject, 'factoryInstances')['SomeClass'] ?? null
);
Copy link
Member

Choose a reason for hiding this comment

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

what is the reason of removing these assertions?

I know that probably these are not the best, as these checks attributes and we really shouldn't do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my Review comment. The test should not check the internal state and it will stop working with PHPUnit 9

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 agree it should not. The problem is that there was no any other way to check it, that's why it was done that way (and we have it in many components....).

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 know, and it was me who added them. We could use Reflection directly, but it still feels kinda hacky and bad.
The question is: do we really need this kind of assertion, or could we do better with infection and benchmark testing here (two tools I'd like to be added anyways).

What do we want to test here? A factory class name should be accepted or more? If more: does it really add value?

That's some topic where I'd like to hear @Ocramius opinion, if he could spend a minute

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michalbundyra I re-added the assertions with reflection, but I still don't think that they add any real value to the test.

}
}