Skip to content

Mention where headers were already sent if session_start fails #16378

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 15, 2024
Merged

Mention where headers were already sent if session_start fails #16378

merged 3 commits into from
Oct 15, 2024

Conversation

NattyNarwhal
Copy link
Member

We had previously improved where sessions were already started, and where headers were already sent when setting headers, but not where a header has been sent if we try to set the header cookie.

Fixes GH-16372

@NattyNarwhal
Mention where headers were already sent if session_start fails
30081d7 
We had previously improved where sessions were already started, and
where headers were already sent when setting headers, but not where a
header has been sent if we try to set the header cookie.

Fixes phpGH-16372
@NattyNarwhal NattyNarwhal requested a review from Girgias as a code owner October 11, 2024 19:59
@NattyNarwhal
Copy link
Member Author

Notes:

  • I'm targeting master because I think this is a new feature, and it's pretty late into 8.4 development.
  • There are a ton more spots beyond session_start, but also regenerating IDs, INI options, etc. that could also distinguish where it has been sent/started. It might be a good idea to fix these too, and also probably the refactor to go along with it.

Girgias
Girgias approved these changes Oct 12, 2024
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this is a nice improvement :)

nielsdos
nielsdos reviewed Oct 12, 2024
View reviewed changes
nielsdos
nielsdos approved these changes Oct 14, 2024
View reviewed changes
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@NattyNarwhal
Copy link
Member Author

I'll merge into master; let me know this should be backported.

@NattyNarwhal NattyNarwhal merged commit edf351c into php:master Oct 15, 2024
8 of 10 checks passed
@NattyNarwhal NattyNarwhal mentioned this pull request Oct 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nielsdos nielsdos nielsdos approved these changes

@Girgias Girgias Girgias approved these changes

Assignees
No one assigned
Labels
Extension: session
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Session cannot be started .. Unclear / Incomplete warning message
3 participants
@NattyNarwhal @nielsdos @Girgias