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

Fixes from typehints of the code #2466

Merged
merged 11 commits into from
Dec 21, 2019
Merged

Fixes from typehints of the code #2466

merged 11 commits into from
Dec 21, 2019

Conversation

gmponos
Copy link
Member

@gmponos gmponos commented Dec 15, 2019

No description provided.

@gmponos
Copy link
Member Author

gmponos commented Dec 15, 2019

Hello...

Initially I had committed only this until this commit 1906171 but then I thought that it is not worth creating a separate PR for this.. So I pushed more changes here.

Summary:

  • Add more typehints
  • Add more docblocks.
  • Clears more phpstan issues.

src/MessageFormatter.php Outdated Show resolved Hide resolved
@GrahamCampbell
Copy link
Member

Add an (array) cast at line 72 to fix the phpstan warning.

@gmponos
Copy link
Member Author

gmponos commented Dec 16, 2019

@GrahamCampbell I believe that casting this to an array will hide a possible error.

The error was reported because for some weird reason that I am still investigating I've run the baseline command and it automatically removed those ignore errors although it was on a code that I haven't touched.

I tried the changes here to affect in the least possible way the behavior. So I believe we should see this on a different PR.

phpstan-baseline.neon Outdated Show resolved Hide resolved
@GrahamCampbell
Copy link
Member

The error was reported because for some weird reason that I am still investigating

it's because that function call may return null as well as an array, at line 72.

@gmponos
Copy link
Member Author

gmponos commented Dec 16, 2019

The error was reported because for some weird reason that I am still investigating

it's because that function call may return null as well as an array, at line 72.

@GrahamCampbell That was not my full sentence.

Let me elaborate.. I've run the baseline command locally and for some reason it has removed from the baseline file the ignore error above.. and then I committed the file with the ignored error removed... Hence it was reported on the CI.

Baseline file has tons of errors that you can comment in any PR. Same here.. The comment is out of the scope of my changes.

Never the less you could say it is an error that it must be fixed.. why not do it now? Problem is that I don't know if having to cast with array hides any future error that should be reported.. Personally I would feel more confident having a check if it is null to throw an exception. But again this is out of the scope of the changes I wanted to apply here... I hope this is clear..

I promise that I will be the next first thing that I will try to fix from the baseline.. or maybe you can do it.. but personally I would go with the exception solution.. If @sagikazarmark or @Nyholm also feel that the array cast can be added here safely let me know...

@sagikazarmark
Copy link
Member

Sorry, I'm quite out of context here, so I will leave that to @gmponos and @Nyholm

@Nyholm
Copy link
Member

Nyholm commented Dec 21, 2019

I agree it is an issue. Since @gmponos dont want to fix the existing issue in this PR I guess it is up for grabs for everybody.

@Nyholm Nyholm merged commit 45b360e into guzzle:master Dec 21, 2019
@gmponos gmponos deleted the typehint-fixes branch December 21, 2019 08:58
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.

None yet

4 participants