Skip to content

Conversation

@kamil-tekiela
Copy link
Member

@kamil-tekiela kamil-tekiela commented Jan 19, 2026

The PR changes the signatures of methods in mysqlnd_auth to remove the session_options parameter which is just a shortcut for conn->options. Since conn is available in all of these method signatures, it doesn't make much sense to duplicate the information.

@bukka
Copy link
Member

bukka commented Jan 19, 2026

What's the point of this?

@kamil-tekiela
Copy link
Member Author

Code cleanup.

@TimWolla
Copy link
Member

It's hard to tell the impact based from the diff, commit messages and PR description alone. It doesn't entirely look like a noop, but I'm not familiar with this part of the code.

Either way, please add an entry to UPGRADING.INTERNALS, explaining that the change was made and what actions a developer needs to take of they happen to use the removed API.

@kamil-tekiela kamil-tekiela force-pushed the Clean-up-MYSQLND_SESSION_OPTIONS branch from dad1095 to 8c7ba85 Compare January 20, 2026 00:20
@kamil-tekiela kamil-tekiela changed the title Clean up mysqlnd session options Drop unnecessary parameter in mysqlnd_auth Jan 20, 2026
@kamil-tekiela
Copy link
Member Author

I have added an UPGRADING entry and I have split it up into two PRs for better clarity. #20979

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, this makes sense to me then. Please wait a little for others to check and agree with the removal before merging.

Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

This one is now fine. Don't think it even needs note in UPGRADING internals because it does not change any exported as far as I see.

@kamil-tekiela kamil-tekiela merged commit 3ed4d2a into php:master Jan 20, 2026
10 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.

3 participants