Skip to content

pdo_firebird: Cleanup code. #15510

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 3 commits into from
Aug 27, 2024
Merged

pdo_firebird: Cleanup code. #15510

merged 3 commits into from
Aug 27, 2024

Conversation

sim1984
Copy link
Contributor

@sim1984 sim1984 commented Aug 20, 2024

Since Firebird PDO now requires fbclient 3.0+ to compile, some conditional compilation directives are no longer needed in the code.

Remove unneeded `#if FB_API_VER >= 25`, `#if FB_API_VER >= 30`,
`#ifdef SQL_BOOLEAN`
…4 KB.

The new limit is 10 MB, no one in their right mind would transmit a query of such length.
@mlocati
Copy link
Contributor

mlocati commented Aug 20, 2024

About eed7d2d: what about restoring a dump? In this case, we may have rather big queries, right?

@sim1984
Copy link
Contributor Author

sim1984 commented Aug 20, 2024

No. There is no support INSERT...VALUES with multiple entries. And it is not customary to do dumps there through a script like MySQL. But there may be a large EXECUTE BLOCK or DDL to create a PSQL PACKAGE BODY.
In any case, the error will be reported by fbclient or firebird server itself. There will be no buffer overflow.

@Girgias Girgias requested a review from cmb69 August 20, 2024 23:01
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

While I don't understand the SQL_TEXT to SQL_VARYING conversion that already happens, I trust you on this. Generally, this PR looks good to me.

Note that the Windows CI failure is unrelated (see PR#15518).

@sim1984
Copy link
Contributor Author

sim1984 commented Aug 21, 2024

While I don't understand the SQL_TEXT to SQL_VARYING conversion that already happens, I trust you on this.

This is done for the purpose of unification. For other data types, we also convert them to SQL_TEXT if a string is supplied as a parameter. This code is shorter, and at the same time allows empty strings to be treated as NULL for enumerated data types.

@cmb69
Copy link
Member

cmb69 commented Aug 26, 2024

@SakiTakamachi, if you have no objections, I'll merge this tomorrow.

@SakiTakamachi
Copy link
Member

I had been in poor health for a long time.

This makes sense to me.

@cmb69 cmb69 merged commit 8487ddb into php:master Aug 27, 2024
9 of 10 checks passed
@cmb69
Copy link
Member

cmb69 commented Aug 27, 2024

Get well soon @SakiTakamachi! :)

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