Implemented handling BLOBs represented as stream resources for IBM DB2 #3309

morozov · 2018-10-04T05:51:38Z

Q	A
Type	improvement
BC Break	no
Fixed issues	#3288

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php

morozov · 2018-10-06T04:10:53Z

Please see the update.

Ocramius

Overall good improvement: please consider not squashing your commits though, as I had a hard time remembering what changed here. (/cc @lcobucci exemplary case of why not to squash)

Ocramius · 2018-10-06T11:26:06Z

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php

+        return $handle;
+    }
+
+    private function copyStreamToStream($source, $target) : void


Please document these types

Ocramius · 2018-10-06T11:26:10Z

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php

+        }
+    }
+
+    private function writeStringToStream($string, $target) : void


Please document these types

Ocramius · 2018-10-06T11:26:21Z

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php

+    /**
+     * @throws DB2Exception
+     */
+    private function bind($column, &$variable, int $parameterType, int $dataType) : void


Please document these types

$column should be int|string, right?

Majkl578 · 2018-10-06T13:11:29Z

@Ocramius

please consider not squashing your commits though, as I had a hard time remembering what changed here

GitHub has a feature called "Changes since your last review". GitLab has much better tracking of changes during squashes/rebases though.

Ocramius · 2018-10-06T13:14:17Z

Didn't see any of that in the UI, nor seeing the fixup commits that address feedback.

Yes, as a reviewer, squashes make things harder (also on gitlab, which I use at work).

morozov · 2018-10-08T06:04:19Z

please consider not squashing your commits though, as I had a hard time remembering what changed here.

Point taken. I just hate it when a pull request is merged with all those "working" commits. Given that reviews are attached to a state of the pull request, once I squash an approved patch, it'll lose approval and someone will have to re-approve it or will merge it as is.

morozov · 2018-10-08T06:07:10Z

GitHub has a feature called "Changes since your last review".

Where is it? I heard Gerrit can do something like this. Regardless of how the pull request is structured, it gives you the diff between the previous and current state.

Majkl578 · 2018-10-08T06:28:03Z

But since you didn't force push I don't know how this handles squashes.

Yes, as a reviewer, squashes make things harder (also on gitlab, which I use at work).

Never had this problem in GitLab, incremental diffs between pushes are very helpful there.

morozov · 2018-10-10T15:07:50Z

@Ocramius, please see the update.

Majkl578 · 2018-10-13T18:20:43Z

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php

+    private function copyStreamToStream($source, $target) : void
+    {
+        if (@stream_copy_to_stream($source, $target) === false) {
+            throw new DB2Exception('Could not copy source stream to temporary file: ' . error_get_last()['message']);


Can we somehow test this failure?

Same as above. We could emulate the failure with a mock if streams were objects. Otherwise, we'd have to define an object wrapper for a temporary file and mock it. Too much of a trouble for an implementation detail IMO.

Majkl578 · 2018-10-13T18:20:46Z

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php

+    private function writeStringToStream(string $string, $target) : void
+    {
+        if (@fwrite($target, $string) === false) {
+            throw new DB2Exception('Could not write string to temporary file: ' . error_get_last()['message']);


Can we somehow test this failure?

No, without unnecessarily complicating the API.

Majkl578 · 2018-10-13T18:21:53Z

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php

+        $handle = @tmpfile();
+
+        if ($handle === false) {
+            throw new DB2Exception('Could not create temporary file: ' . error_get_last()['message']);


Hmm, this may be uncoverable code, I have no idea how to simulate tmpfile() failure. :/

The /tmp filesystem suddenly became read-only? Or partition is full. Not sure if we want to bother too much testing it.

Mostly no space left on device, that may happen on tmpfs (RAM-based) /tmp. :)

Yes, but I still don't want to change the API to make it unit-testable.

All concerns have been addressed.

@morozov

Release v2.9.0 This is a minor release of Doctrine DBAL that aggregates over 40 fixes and improvements developed by 18 contributors over the last 5 months. This release includes all changes of the 2.8.x series, as well as feature additions and improvements that couldn't land in patch releases. ## Backwards Compatibility Breaks This doesn't contain any intentional Backwards Compatibility (BC) breaks. ## Deprecations * The usage of `NULL` to specify the absence of an offset in `LIMIT`ed queries is deprecated. Use `0` instead. * It's not recommended to rely on the default length specified by implementations of `Type`. These values are not used by the library and will be removed. * It's not recommended to rely on the string representation of `Type` objects. * Regular-expression based asset filters are deprecated in favor of callback-based as more extensible. * Calling `Statement::fetchColumn()` with an invalid column index is deprecated. * The `dbal:import` CLI command is deprecated. Please use other database client applications for import. Please see details in the [UPGRADE.md](UPGRADE.md) documentation. ## New Features * Added support for MariaDB 10.3. * Added support for Windows authentication for SQL Server. * Added support for column length in index definitions on MySQL. ## Improvements and Fixes * Implemented handling BLOB objects represented as streams in the MySQL (`mysqli`) driver. * Implemented handling BLOB objects represented as streams in the IDM DB2 driver. * DBAL is now continuously tested with the PDO driver for Oracle. * Implemented handling of URLs in master-slave and pooling-shard connection configuration. * The codebase is now fully compatible with the Doctrine Coding Standard v5.0. Total issues resolved: **45** **Deprecations:** - [3244: Deprecated dbal:import CLI command](doctrine#3244) thanks to @morozov - [3253: Deprecated usage of the NULL offset in LIMITed queries](doctrine#3253) thanks to @morozov - [3256: Deprecate Doctrine\DBAL\Types\Type::getDefaultLength()](doctrine#3256) thanks to @Majkl578 - [3258: Deprecate Doctrine\DBAL\Types\Type::&doctrine#95;&doctrine#95;toString()](doctrine#3258) thanks to @Majkl578 - [3316: Deprecated regex-based asset filters](doctrine#3316) thanks to @morozov - [3359: Removed DataAccessTest::testFetchColumnNonExistingIndex() since it covers a bug in PDO](doctrine#3359) thanks to @morozov **New Features:** - [2412: Add mysql specific indexes with lengths](doctrine#2412) thanks to @bburnichon - [3278: Add support for MariaDB 10.3](doctrine#3278) thanks to @javiereguiluz - [3283: MariaDB improvements, support 10.3](doctrine#3283) thanks to @sidz - [3333: Allow windows (userless/passwordless) authentication for SQL Server](doctrine#3333) thanks to @odinsey **Bug Fixes:** - [3355: Implemented comparison of default values as strings regardless of their PHP types](doctrine#3355) thanks to @morozov and @Majkl578 **Improvements:** - [3201: Fix support for URL to account for master-slave and pooling-shard connections](doctrine#3201) thanks to @stof - [3217: Fix that MysqliStatement cannot handle streams](doctrine#3217) thanks to @mpdude - [3235: Use PSR-4 autoloader](doctrine#3235) thanks to @Majkl578 - [3254: Throw ConversionException when unserialization fail for array and object types](doctrine#3254) thanks to @seferov - [3259: Update export ignores](doctrine#3259) thanks to @Majkl578 - [3309: Implemented handling BLOBs represented as stream resources for IBM DB2](doctrine#3309) thanks to @morozov and @mpdude - [3331: Fetch all should use the driver statement's fetchAll method](doctrine#3331) thanks to @michaelcullum **Documentation Improvements:** - [3223: GitHub template grammar/spelling fixes](doctrine#3223) thanks to @GawainLynch - [3232: Removed NOW() from QueryBuilder usage examples](doctrine#3232) thanks to @morozov - [3239: 2.8 in README & branch alias to 2.9](doctrine#3239) thanks to @Majkl578 - [3269: Fixed type hints in DockBlocks](doctrine#3269) thanks to @marforon - [3275: Add .doctrine-project.json to root of the project.](doctrine#3275) thanks to @jwage - [3276: Update homepage](doctrine#3276) thanks to @Majkl578 - [3280: Use behaviuor instead of behavior](doctrine#3280) thanks to @BackEndTea - [3285: Remove old comment from MysqliStatement](doctrine#3285) thanks to @mpdude - [3318: Removed link to www.doctrine-project.org from doc blocks](doctrine#3318) thanks to @morozov - [3319: remove ClassLoader](doctrine#3319) thanks to @garak - [3337: Fix of links in documentation](doctrine#3337) thanks to @SenseException - [3350: Remove pdo&doctrine#95;sqlsrv from known vendor issues list](doctrine#3350) thanks to @ostrolucky - [3357: Fix typo](doctrine#3357) thanks to @BenMorel - [3370: Removed 2.7 from README](doctrine#3370) thanks to @morozov **Code Quality Improvements:** - [3252: Replaced call&doctrine#95;user&doctrine#95;func&doctrine#95;array() of a fixed method with the usage of variadic arguments](doctrine#3252) thanks to @morozov - [3306: Fixed coding standard violations in the codebase](doctrine#3306) thanks to @morozov - [3303: Updated doctrine/coding-standard to 5.0, ](doctrine#3303) thanks to @morozov - [3317: Implemented proper escaping of string literals in platforms and schema managers](doctrine#3317) thanks to @morozov - [3363: Remove redundant implements](doctrine#3363) thanks to @BenMorel **Continuous Integration Improvements:** - [3307: Test against the latest stable sqlsrv extension](doctrine#3307) thanks to @morozov - [3320: Trying to fix failing DB builds](doctrine#3320) thanks to @morozov - [3325: Updated PHPUnit to 7.4](doctrine#3325) thanks to @morozov - [3339: ContinuousPHP configuration for PDO Oracle driver](doctrine#3339) thanks to @morozov - [3365: Reorganize Travis build matrix](doctrine#3365) thanks to @BenMorel # gpg: Signature made Tue Dec 4 05:44:06 2018 # gpg: using RSA key 374EADAF543AE995 # gpg: Can't check signature: public key not found # Conflicts: # README.md # lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php # lib/Doctrine/DBAL/Version.php

morozov added Improvement ibm_db2 Prepared Statements labels Oct 4, 2018

Ocramius suggested changes Oct 4, 2018

View reviewed changes

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php Show resolved Hide resolved

morozov force-pushed the issues/3288 branch from 004f641 to 93981d5 Compare October 4, 2018 06:45

Majkl578 reviewed Oct 4, 2018

View reviewed changes

lib/Doctrine/DBAL/Driver/IBMDB2/DB2Statement.php Outdated Show resolved Hide resolved

morozov force-pushed the issues/3288 branch 2 times, most recently from bfd5472 to 73192f0 Compare October 4, 2018 17:09

Ocramius previously requested changes Oct 6, 2018

View reviewed changes

Majkl578 reviewed Oct 13, 2018

View reviewed changes

Majkl578 previously approved these changes Oct 13, 2018

View reviewed changes

Implemented handling BLOBs represented as stream resources for IBM DB2

7034013

morozov dismissed Majkl578’s stale review via 7034013 October 13, 2018 19:20

morozov force-pushed the issues/3288 branch from fabcfa3 to 7034013 Compare October 13, 2018 19:20

Majkl578 approved these changes Oct 13, 2018

View reviewed changes

morozov merged commit b74a192 into doctrine:master Oct 13, 2018

morozov deleted the issues/3288 branch October 13, 2018 20:09

morozov self-assigned this Oct 13, 2018

morozov added this to the 2.9.0 milestone Oct 13, 2018

morozov mentioned this pull request Oct 13, 2018

DB2 does not support streams for the LARGE_OBJECT parameter type #3288

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented handling BLOBs represented as stream resources for IBM DB2 #3309

Implemented handling BLOBs represented as stream resources for IBM DB2 #3309

morozov commented Oct 4, 2018

morozov commented Oct 6, 2018

Ocramius left a comment

Ocramius Oct 6, 2018

Ocramius Oct 6, 2018

Ocramius Oct 6, 2018

Ocramius Oct 6, 2018

Majkl578 commented Oct 6, 2018

Ocramius commented Oct 6, 2018

morozov commented Oct 8, 2018

morozov commented Oct 8, 2018 •

edited

Majkl578 commented Oct 8, 2018

morozov commented Oct 10, 2018

Majkl578 Oct 13, 2018

morozov Oct 13, 2018

Majkl578 Oct 13, 2018

morozov Oct 13, 2018

Majkl578 Oct 13, 2018

morozov Oct 13, 2018

Majkl578 Oct 13, 2018

morozov Oct 13, 2018

Implemented handling BLOBs represented as stream resources for IBM DB2 #3309

Implemented handling BLOBs represented as stream resources for IBM DB2 #3309

Conversation

morozov commented Oct 4, 2018

morozov commented Oct 6, 2018

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Majkl578 commented Oct 6, 2018

Ocramius commented Oct 6, 2018

morozov commented Oct 8, 2018

morozov commented Oct 8, 2018 • edited

Majkl578 commented Oct 8, 2018

morozov commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented Oct 8, 2018 •

edited