-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Add headers handler in CLI SAPI #16145
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
base: master
Are you sure you want to change the base?
Conversation
To pass with the new behavior.
✋ anybody could review it or start a discussion ?? PD: I can start it later in internals if necessary !! |
So I think this needs an RFC because it changes the way how CLI treats header function. I can see your use case but as can be seen in the issue, there is already one objection to it (note about throwing exception instead) so that alone shows there should be probably RFC. Also changing behaviour is in some way a BC break as you actually needed to change one of the standard test. This would be still acceptable for minor version IMO but it should not state there is no BC break at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about to test these functions as well:
--SKIPIF-- | ||
<?php include "skipif.inc" ?> | ||
--INI-- | ||
expose_php=On |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave the X-Powered-By
header test separately and remove it by default in the others.
<?php | ||
setcookie('hi', time()+3600); | ||
|
||
setcookie('hi'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please duplicate the test with setrawcookie function
We are using the core
<?php
ob_start();
echo PHP_SAPI . PHP_EOL;
http_response_code(400);
echo http_response_code();
/* return
cli
400
*/
<?php
ob_start();
echo PHP_SAPI . PHP_EOL;
session_set_cookie_params([
'lifetime' => 600,
'path' => '/',
'domain' => 'example.com',
'secure' => true,
'httponly' => true,
'samesite' => true
]);
var_dump(session_get_cookie_params());
/* return
cli
array(6) {
["lifetime"]=>
int(600)
["path"]=>
string(1) "/"
["domain"]=>
string(11) "example.com"
["secure"]=>
bool(true)
["httponly"]=>
bool(true)
["samesite"]=>
string(1) "1"
}
*/
|
Wouldn't it better to create a full CLI mirror for every "http"-like function to not get back to another PR/RFC when you want to touch another aspect of the servers? |
Check issue #12304
With this change, we can use the headers from CLI SAPI. But the most important think is that we can also use setcookie() and sessions.
And also it's useful for PHP tests from CLI.
All the included tests pass.

Only fail the actualheader() and friends [ext/standard/tests/general_functions/head.phpt]
that test the old behavior.I can change this test if this PR will be accepted. But now this test show the difference.
Changed.
https://github.com/joanhey/php-src/blob/cli-header-handler/ext/standard/tests/general_functions/head.phpt
BC: Nothing
That functions now do nothing from CLI.
I don't think that this feature need an RFC, but you'll say it.
Thank you