Skip to content

Simplify PDO include paths #14444

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 2 commits into from
Jun 3, 2024
Merged

Simplify PDO include paths #14444

merged 2 commits into from
Jun 3, 2024

Conversation

petk
Copy link
Member

@petk petk commented Jun 2, 2024

PDO include paths can be simplified and synced as done in other extensions: either the project root directory or the phpincludedir (for the system installation). The 'ext' include is automatically appended when doing phpize build. In php-src it is only present on Windows build. The PHP_CHECK_PDO_INCLUDES is left intact working as before and checks if PDO headers are found.

Unless I'm missing something that makes the pdo_* extensions different with these includes.

PDO include paths can be simplified and synced as done in other
extensions: either the project root directory or the phpincludedir (for
the system installation). The 'ext' include is automatically appended
when doing phpize build. In php-src it is only present on Windows build.
The PHP_CHECK_PDO_INCLUDES is left intact working as before and checks
if PDO headers are found.
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.

I think this makes sense, less magic overall :)

Edit: Maybe a UPGRADING.INTERNALS note for external PDO drivers could be useful? 🤔

Copy link
Member

@NattyNarwhal NattyNarwhal left a comment

Choose a reason for hiding this comment

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

ok odbc, agree with needing a note for internals upgrading

@petk
Copy link
Member Author

petk commented Jun 3, 2024

Of course. I can add this to the upgrading file. Except that PDO extensions out there are not affected by this change. There is still the PHP_CHECK_PDO_INCLUDES macro available and creates the pdo_cv_inc_path variable. For example: https://github.com/microsoft/msphpsql/blob/e253bcdee587ccc56fc4f1fe38cc741c4c888551/source/pdo_sqlsrv/config.m4#L31

@petk petk merged commit df481ef into php:master Jun 3, 2024
2 checks passed
@petk petk deleted the patch-pdo-includes branch June 3, 2024 10:56
petk added a commit to petk/php-src that referenced this pull request Sep 10, 2024
This was added as requested in the pull request
php#14444 but actually isn't important
to log like this here. Existing PHP extensions using PDO still have
their own way of adding the -I flag as before and aren't affected by
this change.
@petk
Copy link
Member Author

petk commented Sep 10, 2024

Hello, I'm looking at the UPGRADING.INTERNALS change here again and basically this doesn't need to be noted in it. Because probably this wasn't clear enough back then that this change doesn't affect other PDO extensions out there. I'll remove the change in the changelog if you're all fine with this: #15816

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.

5 participants