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

SetCookie constructor bugfix #3089

Closed
wants to merge 2 commits into from
Closed

Conversation

idealogica
Copy link

@idealogica idealogica commented Dec 5, 2022

The following issue has been fixed:
When using fromString(), for the following kind of incorrectly formed cookie
fr=synced; Max-Age=604800 Expires=Mon, 12 Dec 2022 13:27:50 GMT; Domain=.emxdgt.com; Path=/; SameSite=None; Secure; HttpOnly
it gives A non well formed numeric value encountered in /app/proxy/vendor/guzzlehttp/guzzle/src/Cookie/SetCookie.php notice

The following issue has been fixed:
For the following kind of incorrectly formed cookie
```fr=synced; Max-Age=604800 Expires=Mon, 12 Dec 2022 13:27:50 GMT; Domain=.emxdgt.com; Path=/; SameSite=None; Secure; HttpOnly```
it gives `A non well formed numeric value encountered in /app/proxy/vendor/guzzlehttp/guzzle/src/Cookie/SetCookie.php` notice
@gumbophp
Copy link

gumbophp commented Jan 31, 2023

GetMaxAge withith your introduction moving that up the chain wiill brake test :)
I guess you understand why .

@idealogica
Copy link
Author

GetMaxAge withith your introduction moving that up the chain wiill brake test :) I guess you understand why .

Anyway this case should be handled explicitly to avoid php native errors/notices. Maybe there is a reason to throw an exception in constructor or just set max age to null if the value is incorrect

@GrahamCampbell
Copy link
Member

Thanks for the PR, but I don't feel that this is the correct solution. The return type of getMaxAge is indicated as int|null, so we should probably make it so.

@GrahamCampbell
Copy link
Member

It looks like missing constructor validation is the issue here.

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.

None yet

3 participants