Skip to content
This repository was archived by the owner on Jan 29, 2020. It is now read-only.

Fixes #330 current NULL for mysqli #337

Closed
wants to merge 1 commit into from
Closed

Fixes #330 current NULL for mysqli #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Adapter/Driver/Mysqli/Result.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ class Result implements
protected $nextComplete = false;

/**
* @var bool
* @var mixed
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any stricter type that can be used here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be null or array, depending on the current() state. Moreover, I just reviewed all the adapter results and this is the scenario:

Adapter\Driver\Pgsql - current(): array|false
Adapter\Driver\Mysqli - current(): array|null
Adapter\Driver\Oci8 - current(): array|false
Adapter\Driver\Pdo - current(): array|object|false
Adapter\Driver\Sqlsrv - current(): array|false|null

I think we should try to normalize it. Maybe, we can throw an Exception if the adapter returns an error during the fetch process, and use null as empty value. Of course, this will be released with zend-db 3.0.0
@Ocramius and @willy0275 what do you think?

*/
protected $currentData = false;
protected $currentData = null;
Copy link
Member

Choose a reason for hiding this comment

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

This is a BC break for anything inheriting from this class. Sadly, the class is not final, so these changes land in future releases (develop)

Copy link

Choose a reason for hiding this comment

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

The thing is, 2.9 introduced a BC break from 2.8 because recent previous versions used to return null on empty result sets. I remember more than a year ago I was using false (even though signature said null) and it was "fixed" and I had to change all of my code to use null instead of false. So I think a BC break here from 2.9 would be acceptable to put things back to "normal". If not, I'll have to wait for 3.0 for a "simple" bug fix. I can't even go back to 2.8 because there are requirements that are not met with PHP 7.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm planning it for zend-db 3.0.0. I'll point to develop, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to release this in 2.10.0 since it's a rollback of 2.8 behaviour.

Copy link

Choose a reason for hiding this comment

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

Thanks! We are still on our local patched version of 2.9, so it's going to be great to be able to switch back to the main repo.


/**
*
Expand Down
32 changes: 1 addition & 31 deletions test/integration/Adapter/Driver/Mysqli/ConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,37 +12,7 @@
class ConnectionTest extends TestCase
{

protected $variables = [
'hostname' => 'TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL_HOSTNAME',
'username' => 'TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL_USERNAME',
'password' => 'TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL_PASSWORD',
'database' => 'TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL_DATABASE',
];

/**
* Sets up the fixture, for example, opens a network connection.
* This method is called before a test is executed.
*/
protected function setUp()
{
if (! getenv('TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL')) {
$this->markTestSkipped('Mysqli integration test disabled');
}

if (! extension_loaded('mysqli')) {
$this->fail('The phpunit group integration-mysqli was enabled, but the extension is not loaded.');
}

foreach ($this->variables as $name => $value) {
if (! getenv($value)) {
$this->markTestSkipped(sprintf(
'Missing required variable %s from phpunit.xml for this integration test',
$value
));
}
$this->variables[$name] = getenv($value);
}
}
use TraitSetup;

public function testConnectionOk()
{
Expand Down
50 changes: 50 additions & 0 deletions test/integration/Adapter/Driver/Mysqli/TableGatewayTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
<?php

namespace ZendIntegrationTest\Db\Adapter\Driver\Mysqli;

use PHPUnit\Framework\TestCase;
use Zend\Db\Adapter\Adapter;
use Zend\Db\TableGateway\TableGateway;

class TableGatewayTest extends TestCase
{
use TraitSetup;

/**
* @see https://github.com/zendframework/zend-db/issues/330
*/
public function testSelectWithEmptyCurrentWithBufferResult()
{
$adapter = new Adapter([
'driver' => 'mysqli',
'database' => $this->variables['database'],
'hostname' => $this->variables['hostname'],
'username' => $this->variables['username'],
'password' => $this->variables['password'],
'options' => ['buffer_results' => true]
]);
$tableGateway = new TableGateway('test', $adapter);
$rowset = $tableGateway->select('id = 0');

$this->assertNull($rowset->current());
}

/**
* @see https://github.com/zendframework/zend-db/issues/330
*/
public function testSelectWithEmptyCurrentWithoutBufferResult()
{
$adapter = new Adapter([
'driver' => 'mysqli',
'database' => $this->variables['database'],
'hostname' => $this->variables['hostname'],
'username' => $this->variables['username'],
'password' => $this->variables['password'],
'options' => ['buffer_results' => false]
]);
$tableGateway = new TableGateway('test', $adapter);
$rowset = $tableGateway->select('id = 0');

$this->assertNull($rowset->current());
}
}
43 changes: 43 additions & 0 deletions test/integration/Adapter/Driver/Mysqli/TraitSetup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php
/**
* @see https://github.com/zendframework/zend-db for the canonical source repository
* @copyright Copyright (c) 2005-2018 Zend Technologies USA Inc. (https://www.zend.com)
* @license https://github.com/zendframework/zend-db/blob/master/LICENSE.md New BSD License
*/

namespace ZendIntegrationTest\Db\Adapter\Driver\Mysqli;

trait TraitSetup
Copy link
Member

Choose a reason for hiding this comment

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

Let's please not use traits for stashing out duplicated code: this is static code, and should be kept as such (static utility)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate why? Moreover, you mentioned this is static code?! Thanks.

{
protected $variables = [
'hostname' => 'TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL_HOSTNAME',
'username' => 'TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL_USERNAME',
'password' => 'TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL_PASSWORD',
'database' => 'TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL_DATABASE',
];

/**
* Sets up the fixture, for example, opens a network connection.
* This method is called before a test is executed.
*/
protected function setUp()
{
if (! getenv('TESTS_ZEND_DB_ADAPTER_DRIVER_MYSQL')) {
$this->markTestSkipped('Mysqli integration test disabled');
}

if (! extension_loaded('mysqli')) {
$this->fail('The phpunit group integration-mysqli was enabled, but the extension is not loaded.');
}

foreach ($this->variables as $name => $value) {
if (! getenv($value)) {
$this->markTestSkipped(sprintf(
'Missing required variable %s from phpunit.xml for this integration test',
$value
));
}
$this->variables[$name] = getenv($value);
}
}
}