Skip to content

Declare ext/mysqli constants in stubs #8811

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 5 commits into from
Jul 18, 2022
Merged

Conversation

kocsismate
Copy link
Member

No description provided.

@kamil-tekiela
Copy link
Member

Hi Mate,
I haven't been following this change. Can you please explain to me why are we changing this? What are all the UNKNOWNS in the stub? Why is the IS_MARIA constant in a separate file? Please give me some background on this.

@kocsismate
Copy link
Member Author

Hi Kamil,

I'll do so so a bit later! Thanks!

@kocsismate
Copy link
Member Author

I haven't been following this change. Can you please explain to me why are we changing this? What are all the UNKNOWNS in the stub? Why is the IS_MARIA constant in a separate file? Please give me some background on this.

Not long ago, I added support for declaring global and class constants in stubs. My motivation was to 1.) make the generated class synopsis pages complete by including class constants 3.) make the registration and maintenance of constants easier 2.) expose the name and type of all constants for 3rd party tooling (e.g. PHPStan).

The gotcha is that constant values cannot be declared in stubs as they mostly refer to C constant values (this is the @cname annotation) that's why we have to use the UNKNOWN fake value.

Regarding your question about MYSQLI_IS_MARIADB, its value depends on the surrounding ifdef (https://github.com/php/php-src/pull/8811/files#diff-db1ecb685cb53c1e50dcb08008f5e2439c27b6dc8a8769d9911e99a9bf72c9acL692), so I chose to add a separate MYSQLI_IS_MARIADB macro and refer this value in the stub. I thought that private header file is a good place for doing so, but I don't mind adding it anywhere else.

Copy link
Member

@kamil-tekiela kamil-tekiela left a comment

Choose a reason for hiding this comment

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

I just had the time to study this in more detail today and I finally understood what was going on with that MARIA constant. Please remove it altogether and adjust the test. This constant is impossible in PHP 8.2. We don't use mariadb libmysql anymore.

Everything else seems to be ok.

@kocsismate
Copy link
Member Author

kocsismate commented Jul 9, 2022

I finally understood what was going on with that MARIA constant. Please remove it altogether and adjust the test. This constant is impossible in PHP 8.2. We don't use mariadb libmysql anymore.

At last, I reverted 923920b because of the concerns with your PR.

@kocsismate
Copy link
Member Author

@kamil-tekiela Is my PR good to be merged?

@kamil-tekiela
Copy link
Member

If you don't mind waiting for the resolution of my PR then let's merge this after my PR. Either the constant will be removed altogether or it should be marked as deprecated and its value set to false. I see no reason to define dummy constant MYSQLI_IS_MARIADB in PHP source either way.

@kocsismate
Copy link
Member Author

I've just got rid of the dummy constant. You are right that it is not needed, and I'd like to merge the PR before yours, so that I can do a followup change (rename @cname to @cvalue).

@kocsismate kocsismate merged commit f0d5368 into php:master Jul 18, 2022
@kocsismate kocsismate deleted the const-mysqli branch July 18, 2022 11:00
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.

2 participants