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: Can't check whether I can send a header #1366

Closed
enumag opened this issue Jan 21, 2014 · 13 comments

Comments

Projects
None yet
3 participants
@enumag
Copy link
Contributor

commented Jan 21, 2014

There is a bug here:

if (!$this->httpResponse->isSent()) {
$this->httpResponse->setCode($e instanceof BadRequestException ? ($e->getCode() ?: 404) : 500);
}

The check if (!$this->httpResponse->isSent()) { is not enough to know that it is possible to send a header (or set HTTP code in this case). It can still throw this error:

trigger_error('Possible problem: you are sending a HTTP header while already having some data in output buffer. Try OutputDebugger or start session earlier.', E_USER_NOTICE);
.

@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2014

There should be a method (probably in Nette\Http\Response) which would return bool whether it is ok to send headers. Possibilities:

  1. public checkHeaders method and a boolean parameter whether to throw errors (ugly imho)
  2. new method, but I'm not surehow to name it... maybe canSendHeader?
@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2014

  1. Add new method like isOutputBufferEmpty and then use both !isSend && isOutputBufferEmpty.
@dg

This comment has been minimized.

Copy link
Member

commented Jan 22, 2014

What about property $warnOnBuffer in Response?

@Majkl578

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2014

Warning is useless when you want to check whether you still can send something or not. But might be good addition to what @enumag suggests, method like headersSent/areHeadersSent.

@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2014

@dg that wouldn't solve this issue, but yeah we can add that as well...

@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2014

@Majkl578 headersSent/areHeadersSent - that's what isSent currently does

public function isSent()
{
return headers_sent();
}

@dg

This comment has been minimized.

Copy link
Member

commented Jan 22, 2014

@enumag the point is, that Application can disable $warnOnBuffer.

@Majkl578

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2014

@dg: The point is rather that one wants to prevent such case at first place...
@enumag: Well, the name of isSent is a bit confusing, I'd change its behavior to check output buffer as well.

@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2014

@dg Well I don't really want to do that, I just want to only send the header only if it is safe to do so...
@Majkl578 Yeah, that is a possibility as well. I'm not really sure which way is the best though.

@dg

This comment has been minimized.

Copy link
Member

commented Jan 22, 2014

@Majkl578 how prevent? It is nonsense.

@enumag „I just want to only send the header only if it is safe to do so.“ But it is safe! Look at code, header will be send.

@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2014

@dg What is the purpose of the "Possible problem: you are sending a HTTP header while already having some data in output buffer." warning then? I'm warned when I do it so I assume it isn't safe to do so (due to possible buffer overflow or something).

EDIT: I see now. It will always work on production, however there is no way to prevent the warning in debug mode.

@enumag

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2014

After some more thinking, it's probably fine.

@enumag enumag closed this Jan 22, 2014

@dg

This comment has been minimized.

Copy link
Member

commented Jan 23, 2014

Fine? I think it is issue.

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.