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

remove wrappers in session ext #18017

Merged
merged 2 commits into from
Mar 13, 2025
Merged

Conversation

jorgsowa
Copy link
Contributor

Removes comment wrappers in session ext. Unnecessary noise and AFAIK they are not used anymore.

@jorgsowa jorgsowa requested a review from Girgias as a code owner March 10, 2025 21:12
@jorgsowa jorgsowa marked this pull request as draft March 10, 2025 21:40
@jorgsowa jorgsowa force-pushed the session/remove-wrappers branch from e574e4d to 1c7ce02 Compare March 10, 2025 21:54
@jorgsowa jorgsowa marked this pull request as ready for review March 10, 2025 21:54
@jorgsowa jorgsowa force-pushed the session/remove-wrappers branch from 1c7ce02 to f7cc969 Compare March 10, 2025 22:04
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.

This is mainly cosmetic, and I'm fine with it, but I don't think many other codeowners for their extensions would approve of such a PR.

@jorgsowa
Copy link
Contributor Author

I don't want to push it to other extensions. I want to finish PRs that I started in ext/session, and I wanted to get rid of those.

I added back comments, although I think that they are redundant to the PHP documentation. But it's fine to keep them.

@Girgias
Copy link
Member

Girgias commented Mar 13, 2025

I don't want to push it to other extensions. I want to finish PRs that I started in ext/session, and I wanted to get rid of those.

I added back comments, although I think that they are redundant to the PHP documentation. But it's fine to keep them.

The PHP documentation is not the source of truth, the C implementation is. I also just want to be able to understand what the C code does without hoping the PHP docs are sufficiently detailed.

@Girgias Girgias merged commit 3f3ac4d into php:master Mar 13, 2025
8 of 9 checks passed
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.

None yet

2 participants