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

[HttpFoundation] Revert getClientIp @return docblock #32682

Merged
merged 1 commit into from Aug 4, 2019

Conversation

ossinkine
Copy link
Contributor

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

This PR reverts #22418, see the comment #22418 (comment)

@ossinkine ossinkine changed the base branch from 4.4 to 3.4 July 23, 2019 14:09
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jul 23, 2019
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 25, 2019

I just tried running this and it returned null, with no "undefined" notice:

$r = new Request();

var_dump($r->getClientIp());

@ossinkine
Copy link
Contributor Author

I think the docblock contains a type that must be, but not one that can be.
There is a lot of not null fields in Request class which will be null after new Request().

@ostrolucky
Copy link
Contributor

No, docblock must specify type it returns

@ossinkine
Copy link
Contributor Author

But now it is not so, we must be consistent. If we want the docblock to contain what the method returns, we need to add |null to other fields and check that the field is not null when we call the getter, but we don’t do that, it is a huge number of redundant checks.

@fabpot
Copy link
Member

fabpot commented Aug 4, 2019

Thank you @ossinkine.

@fabpot fabpot merged commit 7568d34 into symfony:3.4 Aug 4, 2019
fabpot added a commit that referenced this pull request Aug 4, 2019
…nkine)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpFoundation] Revert getClientIp @return docblock

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against branch 4.4.
 - Legacy code removals go to the master branch.
-->

This PR reverts #22418, see the comment #22418 (comment)

Commits
-------

7568d34 [HttpFoundation] Revert getClientIp @return docblock
This was referenced Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants