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

Add headers handler in CLI SAPI #16145

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

joanhey
Copy link
Contributor

@joanhey joanhey commented Sep 30, 2024

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.
image

Only fail the actual header() 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
image

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

To pass with the new behavior.
@joanhey
Copy link
Contributor Author

joanhey commented Dec 2, 2024

✋ anybody could review it or start a discussion ??

PD: I can start it later in internals if necessary !!

@bukka
Copy link
Member

bukka commented Dec 8, 2024

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.

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.

2 participants