standard: various nit and refactorings for standard stream wrappers - #22875
standard: various nit and refactorings for standard stream wrappers#22875Girgias wants to merge 4 commits into
Conversation
| } | ||
| /* check for control characters in login, password & path */ | ||
| if (strncasecmp(new_path, "http://", sizeof("http://") - 1) || strncasecmp(new_path, "https://", sizeof("https://") - 1)) { | ||
| if (zend_string_starts_with_literal_ci(new_path, "http://") || zend_string_starts_with_literal_ci(new_path, "https://")) { |
There was a problem hiding this comment.
it seems the old condition was always true regardless ? I would either drop the "migrated" version of it or
if (!zend_string_starts_with_literal_ci(new_path, "http://") && !zend_string_starts_with_literal_ci(new_path, "https://"))
```. wdyt ?
There was a problem hiding this comment.
I'm... not even sure what the semantics are. Reading the rest of the code it does seem to me that it expects the path to start with an HTTP protocol schema....
As we are passing new_path to php_stream_url_wrap_http_ex
|
Seems your PR resolves a relative Location (e.g. Location: baz on /foo/bar) to /foo/baz instead of master's /foo//baz, because it no longer inserts a / separator after a path prefix that already ends in one. |
So I guess this PR fixes another pre-existing bug as well, and clearly this part of the stream wrapper is barely used and badly tested :/ |
- Use newer zend_string APIs - Make logic more explicit and understandable - Prevent some strlen() recomputations
88c225c to
766bdd8
Compare
2 participants
No description provided.