Skip to content

pdo_firebird: Cleanup code. - #15510

Merged
cmb69 merged 3 commits into
php:masterfrom
sim1984:master
Aug 27, 2024
Merged

pdo_firebird: Cleanup code.#15510
cmb69 merged 3 commits into
php:masterfrom
sim1984:master

Conversation

@sim1984

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

@sim1984

sim1984 commented Aug 20, 2024

Copy link
Copy Markdown
ContributorAuthor

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 cmb69August 20, 2024 23:01

@cmb69cmb69 left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

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
Copy Markdown
Member

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

@SakiTakamachi

Copy link
Copy Markdown
Member

I had been in poor health for a long time.

This makes sense to me.

@cmb69
cmb69 merged commit 8487ddb into php:masterAug 27, 2024
@cmb69

Copy link
Copy Markdown
Member

Get well soon @SakiTakamachi! :)

Sign up for freeto 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

@sim1984@mlocati@cmb69@SakiTakamachi