Skip to content

Conversation

devnexen
Copy link
Member

@devnexen devnexen commented Oct 1, 2024

Those are meant to have 1 or plus values separated by a comma even
if the client set them separately.

@devnexen devnexen linked an issue Oct 1, 2024 that may be closed by this pull request
@devnexen devnexen marked this pull request as ready for review October 2, 2024 05:04
memcpy(ZSTR_VAL(newval) + ZSTR_LEN(curval), ", ", 2);
memcpy(ZSTR_VAL(newval) + ZSTR_LEN(curval) + 2, ZSTR_VAL(client->current_header_value), ZSTR_LEN(client->current_header_value) + 1);

ZVAL_STR(&tmp, newval);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use ZVAL_NEW_STR here, but not terribly important.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is right as far as I understand. But maybe wait for like at least one more approval.

@devnexen devnexen requested a review from cmb69 October 3, 2024 19:10
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the nits, this looks good to me.

* `Set-Cookie` HTTP header being the exception, they can have 1 or more values separated
* by a comma while still possibly be set separately by the client.
**/
if ((entry == NULL && with_comma) || !with_comma) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you revert the conditions, you can simplify:

Suggested change
if ((entry == NULL && with_comma) || !with_comma) {
if (!with_comma || entry == NULL) {

This is the same as with_comma → entry == NULL (logical implication).

**/
if ((entry == NULL && with_comma) || !with_comma) {
ZVAL_STR(&tmp, client->current_header_value);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous empty line:

Suggested change

Those are meant to have 1 or plus values separated by a comma even
 if the client set them separately.
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.

Built-in webserver not passing header correctly

4 participants