Skip to content

Deprecate mysqli_refresh #11929

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 1 commit into from
Aug 9, 2024
Merged

Deprecate mysqli_refresh #11929

merged 1 commit into from
Aug 9, 2024

Conversation

kamil-tekiela
Copy link
Member

@kamil-tekiela kamil-tekiela commented Aug 9, 2023

This functions was using the binary protocol command COM_REFRESH which is deprecated as of MySQL 5.7. We should not be using deprecated commands, so let's deprecate this function which has very easy alternatives in plain text SQL. See:

We didn't have any tests for it so there was nothing to adjust.

@Girgias
Copy link
Member

Girgias commented Aug 10, 2023

To land this in master/PHP 8.3 this needs RM approval @bukka @ericmann @adoy

I am otherwise in favour but this might also require a thread on the internals mailing list to see if anyone complains about this.

@Girgias Girgias mentioned this pull request Aug 10, 2023
@kamil-tekiela
Copy link
Member Author

It can also go in 8.4. I am fine either way.

@bukka
Copy link
Member

bukka commented Aug 11, 2023

yeah agreed better to target 8.4 as this should have some discussion most likely.

@jorgsowa
Copy link
Contributor

IMO, it needs corresponding information in the changelog, because it's either way deprecation.

@kamil-tekiela
Copy link
Member Author

I have added this to PHP 8.4 deprecation list https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_mysqli_refresh

While I haven't included the 9 constants that are used with this function in the PR, I wonder if the constants should also be marked as deprecated. They will be removed when this function gets removed in PHP 9.0, but it's unlikely they have ever been used for anything other than this function.

@Girgias
Copy link
Member

Girgias commented Jan 22, 2024

I have added this to PHP 8.4 deprecation list https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_mysqli_refresh

While I haven't included the 9 constants that are used with this function in the PR, I wonder if the constants should also be marked as deprecated. They will be removed when this function gets removed in PHP 9.0, but it's unlikely they have ever been used for anything other than this function.

Makes sense, maybe just mention the constants in the RFC text?

@TimWolla
Copy link
Member

TimWolla commented Aug 4, 2024

Should be rebased onto master and the use of @deprecated replaced by the #[\Deprecated(since: '8.4', message: 'some explanation')] attribute.

@Girgias
Copy link
Member

Girgias commented Aug 9, 2024

@kamil-tekiela could you rebase this?

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

@Girgias Girgias merged commit 42336e1 into php:master Aug 9, 2024
10 of 11 checks passed
@kamil-tekiela kamil-tekiela deleted the COM_REFRESH branch August 9, 2024 15:45
@jrfnl
Copy link
Contributor

jrfnl commented Aug 12, 2024

Looking at the commit, it looks like the functions have been deprecated, but the constants have not been (though are unlikely to be used with anything but these functions).

The RFC vote question was, however, "Deprecate mysqli_refresh(), mysqli::refresh() and related constants?", which leads me to believe the constants should also be formally deprecated.

I can see some discussion about this above, but would like to make sure anyway.

Was there a particular reason not to deprecate the constants ?

@Girgias
Copy link
Member

Girgias commented Aug 12, 2024

They were indeed, let me do this in a follow-up PR.

EDIT: Kamil beat me to it: #15358

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.

6 participants