Skip to content

Warning to error promotion for set(raw)cookie() - #5819

Closed
Girgias wants to merge 3 commits into
php:masterfrom
Girgias:setcookie-warning-to-error
Closed

Warning to error promotion for set(raw)cookie()#5819
Girgias wants to merge 3 commits into
php:masterfrom
Girgias:setcookie-warning-to-error

Conversation

@Girgias

Copy link
Copy Markdown
Member

I've merged setcookie() and setrawcookie() into a common implementation at the same time

@Girgias
Girgiasforce-pushed the setcookie-warning-to-error branch 2 times, most recently from c1691bf to 987ca60CompareJuly 11, 2020 18:00
@Girgias

Copy link
Copy Markdown
MemberAuthor

If there are no objections I'll merge this at the end of the week.

Comment threadext/standard/head.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cc @kocsismate, but I don't think this format makes sense. $expires_or_options["path"] is not the parameter name.

@nikicnikicJul 23, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWIW, I don't think we should try too hard to get the argument error formatting everywhere. If it's inconvenient from the implementation POV, don't do it. Here, it would be more elegant to keep checks inside php_setcookie.

@kocsismatekocsismateJul 23, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we used unserialize(): "max_depth" option must be of type int, %s given in a similar situation :)

Comment threadext/standard/head.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This RETURN_THROWS() doesn't make much sense, as it cannot be executed after the goto cleanup.

Comment threadext/standard/head.c Outdated
@Girgias
Girgiasforce-pushed the setcookie-warning-to-error branch 2 times, most recently from 399ee62 to 39d24b4CompareJuly 27, 2020 21:12
@Girgias
Girgiasforce-pushed the setcookie-warning-to-error branch from 39d24b4 to 59e239cCompareSeptember 2, 2020 15:35
@Girgias
Girgiasforce-pushed the setcookie-warning-to-error branch from 59e239c to 6521060CompareSeptember 4, 2020 14:34
Comment threadext/standard/head.c
}

PHPAPI int php_setcookie(zend_string *name, zend_string *value, time_t expires, zend_string *path, zend_string *domain, int secure, int httponly, zend_string *samesite, int url_encode)
#define ILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", and \"\\014\""

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
#defineILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", and \"\\014\""
#defineILLEGAL_COOKIE_CHARACTER "\",\", \";\", \" \", \"\\t\", \"\\r\", \"\\n\", \"\\013\", or \"\\014\""

I think...

Comment threadext/standard/head.c
}
// To go above year 9999: 60 * 60 * 24 * 365 * 9999
try {
setcookie('name', 'value', 315328464000);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
setcookie('name', 'value', 315328464000);
setrawcookie('name', 'value', 315328464000);

@Girgias
Girgias deleted the setcookie-warning-to-error branch September 8, 2020 12:30
Sign up for freeto 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.

4 participants

@Girgias@nikic@kocsismate@carusogabriel