Skip to content

Don't export php_pdo_int.h #15688

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

Closed
wants to merge 1 commit into from
Closed

Don't export php_pdo_int.h #15688

wants to merge 1 commit into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Sep 1, 2024

This is, as the name and a comment in the header imply, an internal header which is not supposed to be used by extensions other than PDO (not even by drivers).

Since there is apparently no need to include this header in the parsers of the drivers, we remove these includes, and no longer declare the header to be installed. Given that the header is only exported for a couple of weeks[1], this is not considered to be a BC break, because it's unlikely that external drivers have already been adjusted to use this header, and otherwise they can still be fixed; PHP 8.4 is still in the pre-release stage.

[1] #14797

This is, as the name and a comment in the header imply, an internal
header which is not supposed to be used by extensions other than PDO
(not even by drivers).

Since there is apparently no need to include this header in the parsers
of the drivers, we remove these includes, and no longer declare the
header to be installed.  Given that the header is only exported for a
couple of weeks[1], this is not considered to be a BC break, because
it's unlikely that external drivers have already been adjusted to use
this header, and otherwise they can still be fixed; PHP 8.4 is still in
the pre-release stage.

[1] <php#14797>
Copy link
Member

@petk petk left a comment

Choose a reason for hiding this comment

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

All these related pdo extensions build fine also with phpize. Redundant header indeed. Thanks for catching this. Yes, this is hardly a BC break since this is an internal header. Unless something got missed and something needs it in the parsers. Fine with me anyway.

@cmb69
Copy link
Member Author

cmb69 commented Sep 1, 2024

C:\php\php-8.4.0alpha4-devel-vs17-x64\include>dir /b /s *_int.h
C:\php\php-8.4.0alpha4-devel-vs17-x64\include\ext\gmp\php_gmp_int.h
C:\php\php-8.4.0alpha4-devel-vs17-x64\include\ext\pdo\php_pdo_int.h
C:\php\php-8.4.0alpha4-devel-vs17-x64\include\ext\standard\php_dir_int.h
C:\php\php-8.4.0alpha4-devel-vs17-x64\include\main\streams\php_streams_int.h
C:\php\php-8.4.0alpha4-devel-vs17-x64\include\Zend\zend_strtod_int.h

Regarding php_gmp_int.h; this is exported as of e05cba0. In this case the "int" is likely an abbreviation for "interface", but at least to me that seems to be uncommon for C headers (where "int" usually stands for "internal" to my knowledge), but might be common for C++.

The other headers (php_dir_int.h, php_streams_int.h, zend_strtod_int.h) may or may not be meant to be internal headers, and may be exported inadvertently, because we install ext/standard/*.h, main/streams/*.h and Zend/*.h.

Might be in order to revisit this for PHP 9; I don't think introducing a BC break for these long-standing issues would be appropriate for a minor PHP version.

@cmb69 cmb69 closed this in a57ce05 Sep 1, 2024
@cmb69 cmb69 deleted the cmb/php_pdo_int.h branch September 1, 2024 11:34
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