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

Response, IResponse: unified API (BC break) #148

Closed
wants to merge 1 commit into from

Conversation

xificurk
Copy link

@xificurk xificurk commented Dec 3, 2018

  • bug fix / new feature? a bit of both
  • BC break? yes
  • doc PR: don't think it's needed

Response API at the moment slightly differs from IResponse. This PR unifies both APIs. Also, explicitly marking all nullable parameters as nullable in parameter type hint.

@xificurk
Copy link
Author

@dg Any thoughts? I think this needs to be resolved before stable 3.0 version.

@dg
Copy link
Member

dg commented Feb 28, 2019

?type $var = null is inconsistent with the rest of framework, where is only type $var = null.

@xificurk
Copy link
Author

I always assumed this was a hack for declaring nullable parameters from pre 7.1 times.

@dg
Copy link
Member

dg commented Feb 28, 2019

For PHP both declarations are identical. We can agree to change type $var = null => ?type $var = null in the whole framework, but I think it is unnecessary.

dg added a commit that referenced this pull request Feb 28, 2019
@xificurk
Copy link
Author

xificurk commented Feb 28, 2019

OK, but 398dc22 is not enough, there is still a major incompatibility:
Response::setHeader(string $name, ?string $value) vs IResponse::setHeader(string $name, string $value), plus IResponse::deleteHeader() is completely missing.
And since it's currently not possible to require Response by specific type, it means there is no way to delete remove header.
That was actually the original motivation for this PR. I've got failing builds, at first because I was relying directly on Response, which was deprecated in #90. After replacing the specific implementation by interface, the build started to fail because static analysis (correctly) flagged calls IResponse::setHeader('Foo', null) or IResponse::deleteHeader('Foo') as an error.

@dg
Copy link
Member

dg commented Feb 28, 2019

I understand where problem is and #147 is opened.

There will certainly be much more similar issues in Nette and I'll try to fix them in the next version.

@xificurk
Copy link
Author

#147 revolves around IRequest vs Request API. This PR was about IResponse vs Response API, but the root cause is the same - BC break from #90.

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.

2 participants