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

Fix #78929: Fix a cookie parsing value. Switch to a php_raw_url_decode() #4989

Closed
wants to merge 9 commits into from

Conversation

kachalinalexey
Copy link
Contributor

  1. Switch from cookie parsing function from php_url_decode()
    to php_raw_url_decode(
    ). Only for parsing value.

  2. Move redundant code that doesn't depend from existence of the value
    before and after condition. Execution flow wasn't changed.

  3. Added comment about RFC.

Bug report 78929.
https://bugs.php.net/bug.php?id=78929

@cmb69 cmb69 added the Bug label Dec 11, 2019
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.

The current PR would change the behavior of other input parameters as well; I don't think this is intended. Also, please provide a PHPT test case, if possible. And I would suggest that you rebase onto PHP-7.3 or later (7.2 and older are already out of active support).

Thanks.

@kachalinalexey
Copy link
Contributor Author

Could you please suggest a directory for tests? Is a 'php-src/tests/basic/' directory suitable?

Switch from cookie parsing function from php_url_decode(***)
to php_raw_url_decode(***). Only for parsing value.
Move redundant code that doesnt's depend from existence of the value
before and after condition. Execution flow wasn't changed.
Added comment about RFC.
Bug report 78929.
@cmb69
Copy link
Member

cmb69 commented Dec 12, 2019

Yes, I think 'php-src/tests/basic/' is suitable.

@cmb69 cmb69 changed the base branch from PHP-7.1.30 to PHP-7.3 December 12, 2019 09:42
@cmb69 cmb69 changed the base branch from PHP-7.3 to PHP-7.4 December 12, 2019 09:43
@cmb69
Copy link
Member

cmb69 commented Dec 12, 2019

It seems you didn't rebase, but rather merged, so I rebased onto PHP-7.4 now, but it's still not quite correct. Can you please fix?

@kachalinalexey
Copy link
Contributor Author

All mentioned fixes was implemented.
Thank you for your assistance.

@kachalinalexey kachalinalexey requested a review from cmb69 December 12, 2019 11:37
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.

Thanks! Patch is fine, but for C89 compatibility, I've moved the declarations upwards (wouldn't be needed for PHP 8 anymore, though), and also removed the comment. I also squashed the commits, and tweaked the commit message (including a reference to the RFC section).

@cmb69
Copy link
Member

cmb69 commented Dec 12, 2019

Applied as 79376ab. Thanks again!

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.

4 participants