Skip to content

Deprecate mysqli_kill #11926

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

Merged
merged 4 commits into from
Aug 9, 2024
Merged

Conversation

kamil-tekiela
Copy link
Member

MySQL 5.7 has deprecated COM_PROCESS_KILL which is used under the hood by mysqli. See:

I understand it may be controversial for PHP, but we shouldn't support commands that are deprecated in MySQL. The only problem with this is that the way that PHP tests used this function is actually problematic. When we use mysqli_kill to kill our own process there's no error, but when we use SQL KILL then we get an error about aborted query. It makes sense, but creates a problematic upgrade path. Not sure if there's a way to deal with this.

We could emulate that command in mysqlnd, but that's an undesirable solution.

@Girgias
Copy link
Member

Girgias commented Aug 10, 2023

Same comment as #11929

@DeveloperRob
Copy link

On this, it seems the functionality isn't deprecated in MariaDB, as far as I can see from a quick search anyway. It doesn't mean it needs to be kept in PHP of course (and if the behaviour will differ based on server version, potentially that is justification for its removal by itself), but it does seem there are some subtle differences between $mysqli->kill() and $mysqli->query("KILL ..."); - it isn't quite a straight like for like replacement.

@Girgias
Copy link
Member

Girgias commented Aug 9, 2024

@kamil-tekiela could you rebase this PR?

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.

LGTM if CI is green

@kamil-tekiela kamil-tekiela force-pushed the Deprecate-mysqli_kill branch from 9235de7 to 562f184 Compare August 9, 2024 16:35
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.

Actually, I'm confused why there is an SPL arginfo file change

@kamil-tekiela
Copy link
Member Author

Actually, I'm confused why there is an SPL arginfo file change

You know, I actually didn't see it there. I just run the gen_stub and committed. I'll remove it from this PR.

@kamil-tekiela kamil-tekiela force-pushed the Deprecate-mysqli_kill branch from 562f184 to 346d75b Compare August 9, 2024 16:48
@kamil-tekiela kamil-tekiela merged commit 7801f40 into php:master Aug 9, 2024
10 checks passed
@kamil-tekiela kamil-tekiela deleted the Deprecate-mysqli_kill branch August 9, 2024 17:50
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.

4 participants