Skip to content

Conversation

ashnazg
Copy link
Contributor

@ashnazg ashnazg commented Oct 12, 2015

Tests to show bug behavior of "LOB read loop ends early for multibyte strings", as described in https://bugs.php.net/bug.php?id=70700.

Additional notes are on my question here (http://stackoverflow.com/questions/33026617/pdo-oci-truncates-large-multibyte-clobs) that might help diagnose the bug.

@cjbj: I had to construct this PHPT by hand from my standalone PHP test file, so it may need some polishing to run just right with your test scaffolding (those require targets etc). I'll update PR if you'll highlight any edits that are needed to get it running.

@ashnazg
Copy link
Contributor Author

ashnazg commented Oct 12, 2015

I think it's somewhat plausible that this bug behavior here could be the root cause behind the bug#60994 (#1566).

@krakjoe
Copy link
Member

krakjoe commented Jan 7, 2017

@cjbj please can we get some feedback ?

@cjbj
Copy link
Contributor

cjbj commented Jan 25, 2017

@ashnazg See the updates in https://bugs.php.net/bug.php?id=70700 about why the testcase is not correct.
If you make the changes suggested (and update the comments), we'd be happy to add it to the test suite.
In summary, the LOBs reads are reading characters (not bytes) so the number of bytes of data is greater than the number of bytes being written out. This leads to apparent truncation. The fwrites should be changed to be like fwrite($fh, $data, strlen($data));

@@ -0,0 +1,179 @@
--TEST--
Bug #70700 (LOB read loop ends early for multibyte strings)
Copy link
Contributor

@cjbj cjbj Jan 27, 2017

Choose a reason for hiding this comment

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

Since bug #70700 is not a code bug perhaps the title should just say 'Tests for LOBS with multibyte strings'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Curb service :-)

$target_dbs = array('oracledb' => true, 'timesten' => false); // test runs on these DBs
require(dirname(__FILE__).'/skipif.inc');
?>
--FILE--

Choose a reason for hiding this comment

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

Please add the following between line 11 and 12:
--ENV--
NLS_LANG=.AL32UTF8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that dot supposed to be there? =.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes the dot should be there. It is the separator from the language & territory component. In this case the language & territory are empty meaning the defaults are used. Other examples exist in PHP tests: http://lxr.php.net/xref/PHP-MASTER/ext/oci8/tests/bug47281.phpt#17



echo PHP_EOL, 'Test 1: j', PHP_EOL;
$string1 = 'abc' . str_repeat('j', 10000) . 'xyz';

Choose a reason for hiding this comment

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

The repeat time should be 1000000 to match the expected result.

@krakjoe
Copy link
Member

krakjoe commented Feb 22, 2017

@cjbj can we get another review here please ?

Copy link

@tianfyang tianfyang left a comment

Choose a reason for hiding this comment

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

Thanks for your contributions. I've tested the testcase on different branches. After adding the check of mbstring, we're good to go.

Chuck Burgess
ashnazg@php.net
--SKIPIF--
<?php

Choose a reason for hiding this comment

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

Please add the following check here:
if (!extension_loaded('mbstring')) die('skip mbstring is not enabled');

After adding this, we're good to go.

@php-pulls
Copy link

Comment on behalf of sixd at php.net:

Merged - thanks!

@php-pulls php-pulls closed this Jun 20, 2017
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.

6 participants