Skip to content

Fix bug #54379 PDO_OCI: UTF-8 output gets truncated - #2276

Closed
gureedo wants to merge 1 commit into
php:masterfrom
gureedo:bug54379
Closed

Fix bug #54379 PDO_OCI: UTF-8 output gets truncated#2276
gureedo wants to merge 1 commit into
php:masterfrom
gureedo:bug54379

Conversation

@gureedo

Copy link
Copy Markdown

Replacement for issue #542

@KalleZ

Copy link
Copy Markdown
Member

Tagging Christopher as he is the Oracle guy; @cjbj

@nikic

Copy link
Copy Markdown
Member

Ping @cjbj

@KalleZ

Copy link
Copy Markdown
Member

@nikic@cjbj is currently on holiday, he said he would have a look once he returns

@cjbj

cjbj commented Jan 13, 2017

Copy link
Copy Markdown
Contributor

I'm back; I've asked a developer to look at this. It's in a big queue of tasks TBD.

S->cols[colno].datalen *= 3;
}

S->cols[colno].datalen *= S->H->max_char_width;

@cjbjcjbjJan 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lines 602-606 need to be:

 } else if (dtype == SQLT_BIN){
S->cols[colno].datalen = (ub4) col->maxlen * 2; // raw characters to hex digits
} else {
S->cols[colno].datalen = (ub4) (col->maxlen * S->H->max_char_width);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rationale from the developer who looked at it is:
"Given raw data(SQLT_BIN) is defined as SQLT_CHR (See line #609), raw is returned as strings. So 1 byte raw is represented as two bytes of hex digits.
For example, a 1-byte raw data 'FF' is converted to two 'F' characters. So we need to multiply by 2 (not 3) to accommodate the translation of raw to hex digits. max_char_width should be used for character strings.

Apart from that, other changes all look good including the testcase. The test would normally be skipped in the test suite as a UTF-8 DSN is required.

But we can see the skip reason in the test result and run it manually if required."

@cjbj

cjbj commented Mar 2, 2017

Copy link
Copy Markdown
Contributor

No update from filer on this. But no fear! We've got a corrected patch ready for testing and merge. I'll close this PR

@php-pulls

Copy link
Copy Markdown

Comment on behalf of sixd at php.net:

We will commit a corrected patch

php-pulls pushed a commit that referenced this pull request Mar 2, 2017
This was a modification of an incomplete PR #2276.
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.

7 participants

@gureedo@KalleZ@nikic@cjbj@php-pulls@smalyshev@krakjoe