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

Redundant status code emits? #14

Closed
mindplay-dk opened this Issue Apr 12, 2019 · 4 comments

Comments

Projects
None yet
3 participants
@mindplay-dk
Copy link

commented Apr 12, 2019

I noticed the HTTP status code gets applied with every individual header() call:

https://github.com/zendframework/zend-httphandlerrunner/blob/master/src/Emitter/SapiEmitterTrait.php#L84-L88

But headers get emitted before the status line:

https://github.com/zendframework/zend-httphandlerrunner/blob/master/src/Emitter/SapiEmitter.php#L28-L29

And the status line gets emitted last, with the status code:

https://github.com/zendframework/zend-httphandlerrunner/blob/master/src/Emitter/SapiEmitterTrait.php#L60-L65

Any reason why it isn't enough to just apply the status code last with emitStatusLine()?

Note that this isn't an issue with the code, merely a bit of a comprehension difficulty.

I'm sure it isn't harmful to apply the status code repeatedly - but unless it's meaningful, it's probably confusing more than it is helpful to a person reading the code?

@weierophinney

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

It's to fix a subtle PHP issue where it overrides the status code when certain headers are emitted: zendframework/zend-diactoros#273

@mindplay-dk

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

Yes, I understand the reason, but according to @lcobucci, merely reversing the two lines fixes it.

My question is, is it really necessary to pass the status-code for every individual header-line?

From the rest of the comments, I expect the answer is no?

If I understand correctly, you're defensively setting the status code in both places, in case somebody were to think it's a good idea to change the call order in the other function someday?

(This isn't meant as a criticism or anything - I am genuinely trying to understand how this works.)

@lcobucci

This comment has been minimized.

Copy link

commented Apr 13, 2019

@mindplay-dk you're correct when you say that only changing the order would address the issue. I opted to go with the same approach as symfony/http-foundation does to indeed be more defensive.

I've tried to explain that in the commit messages but reading them now I realise that the explanation could have been better. Sorry about the confusion 😅

@mindplay-dk

This comment has been minimized.

Copy link
Author

commented Apr 15, 2019

@lcobucci thanks, no problem :-)

(I personally wouldn't have done it if it wasn't necessary - most likely, I would have chosen to simply fold the status line and header emits into a single method, so that the order can't be accidentally changed. In my opinion, doing unnecessary things only raises questions or creates confusion. Just my opinion.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.