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.

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

Jump to bottom

Container::getByType fixed phpdoc return typehint #167

Merged
merged 1 commit into from
Sep 17, 2018
Merged

Container::getByType fixed phpdoc return typehint #167

merged 1 commit into from
Sep 17, 2018

Conversation

adaamz
Copy link
Contributor

@adaamz adaamz commented Jul 4, 2018 •
edited
Loading

  • bug fix
  • BC break? no

Container::getByType really returns null (previously "returned" void).

@adaamz adaamz changed the title Container: added typehints Container::getByType fixed phpdoc return typehint Jul 4, 2018
@adaamz adaamz force-pushed the patch-1 branch 2 times, most recently from 9b06abe to 8ff952d Compare July 4, 2018 12:13
dg
dg reviewed Jul 4, 2018
View reviewed changes
src/DI/Container.php Outdated
@@ -251,7 +253,7 @@ public function createInstance(string $class, array $args = [])

/**
* Calls all methods starting with with "inject" using autowiring.
* @param object $service
* @param object $service
Copy link
Member

Choose a reason for hiding this comment

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

This was ok, or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I can revert it if you want... or split to 2nd commit

Copy link
Member

Choose a reason for hiding this comment

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

Ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

splitted to 2nd commit and removed some other unnecessary spaces in phpdocs

Copy link
Member

Choose a reason for hiding this comment

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

These spaces are used intentionally and across all libraries.

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 don't know why to add unnecessary spaces, but it is your opinion. Commit reverted.

@dg dg force-pushed the master branch 3 times, most recently from 8224b32 to d51ca9e Compare September 16, 2018 21:16
@adaamz adaamz force-pushed the patch-1 branch 2 times, most recently from 3779d89 to 9b5c815 Compare September 17, 2018 13:46
@enumag
Copy link
Contributor

enumag commented Sep 17, 2018

Oh great, now PHPStan will fail on every single usage of Container::getByType() because it will think that the method can return null. This is why boolean $throw parameters are evil.

@adaamz
Copy link
Contributor Author

adaamz commented Sep 17, 2018 •
edited
Loading

@enumag I think there is return type extension already... Do you have phpstan/phpstan-nette included?

If it is not there already I can create it, pls create issue.

@dg dg merged commit 88e849b into nette:master Sep 17, 2018
dg pushed a commit that referenced this pull request Sep 17, 2018
@adaamz @dg
typo (#167)
7fbf26e
dg pushed a commit that referenced this pull request Sep 17, 2018
@adaamz @dg
typo (#167)
923da3e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dg dg dg left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adaamz @enumag @dg