Skip to content

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Nov 7, 2021

No description provided.

@staabm
Copy link
Contributor Author

staabm commented Nov 7, 2021

the phpdoc change is helpful to make phpstorm (and phpstan) aware of the type beeing returned by getByType.

not sure such a phpdoc change is considered a BC break, as it leads to static-analysis time errors when implemented in downstream code.

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 reports

Method PHPStan\DependencyInjection\Nette\NetteContainer::getByType() should return T of object but returns object.

I dont know how to fix that

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@staabm staabm Nov 8, 2021

Choose a reason for hiding this comment

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

just added the stub. now I get

Method PHPStan\DependencyInjection\Nette\NetteContainer::getByType() should return T of object but returns T of object.

which seems to be a phpstan bug?

btw: nette-di has newer releases and the latest 3.0.11 has a proper phpdoc for this method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now I committed a workaround, which no longer requires the stubfile - since I was not able to resolve the error mentioned in the comment above.

I figured the problem regarding "should return T of object but returns T of object." should be solved in a separate PR...?

Copy link
Member

Choose a reason for hiding this comment

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

i dislike inline @var when there can be a better solution: https://phpstan.org/writing-php-code/phpdocs-basics#inline-%40var

So I'd like the stub file much more. And this PR is fine, it's called "More precise phpdocs in Container".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I reverted the last 3 commits and we are back at the point

Method PHPStan\DependencyInjection\Nette\NetteContainer::getByType() should return T of object but returns T of object.

@staabm staabm changed the title More precise phpdocs in Container-Interface More precise phpdocs in Container-Interface/Classes Nov 8, 2021
@staabm staabm marked this pull request as ready for review November 8, 2021 13:55
Copy link
Member

Choose a reason for hiding this comment

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

I forgot there's already an extension for this method: https://github.com/phpstan/phpstan-src/blob/master/build/PHPStan/Build/ServiceLocatorDynamicReturnTypeExtension.php

So we actually don't need a stub, but to fix this extension. I did it on master: 12973cd

So you can rebase your branch, remove the stub and any unrelated changes, and it should be fine, hopefully.

@ondrejmirtes
Copy link
Member

Looks like we're hitting some weird bug here.

@ondrejmirtes
Copy link
Member

Alright, figured out a working version: 9384783

Thank you.

@staabm staabm deleted the patch-3 branch November 9, 2021 09:44
@staabm
Copy link
Contributor Author

staabm commented Nov 9, 2021

thank for finishing and sorry for opening such a can of worms, for such a "small change".

but I think its worth it.

@ondrejmirtes
Copy link
Member

It is definitely :)

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.

3 participants