Regression in ImportCommand

[morozov]

Q	A
BC Break	no
Version	3.0.0-dev

On develop, the following issue is reported by PHPStan:

------ ----------------------------------------------------------- 
 Line   lib/Doctrine/DBAL/Tools/Console/Command/ImportCommand.php  
------ ----------------------------------------------------------- 
 95     Call to an undefined method                                
        Doctrine\DBAL\Driver\PDOStatement::nextRowset().           
------ ----------------------------------------------------------- 

The issue looks legit. In order to fix the PHPStan job on Travis, I'm going to white-list this issue but it will have to be fixed properly before the release.

[morozov]

Anyone knows how doctrine-dbal dbal:import is supposed to work? According to the help message, it will "Import SQL file(s) directly to Database", however judging by the implementation, it cannot handle
files which have more than one statements. Here's what I'm trying to do:

1. mysql << 'EOF'
   CREATE TABLE `test` (
     `id` int NOT NULL,
     PRIMARY KEY (`id`)
   )
   EOF

2. cat > data.sql << 'EOF'
   INSERT INTO test (`id`) VALUES (1);
   INSERT INTO test (`id`) VALUES (2);
   EOF

3. bin/doctrine-dbal dbal:import data.sql

The above results into:

An exception occurred while executing 'INSERT INTO test (id) VALUES (1);
INSERT INTO test (id) VALUES (2);
':

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'INSERT INTO test (id) VALUES (2)' at line 2

Other import implementations I saw in PHP would parse the file into multiple statements and execute them one by one but this just passes the file contents to the driver as is.

/cc @Ocramius @Majkl578

[Ocramius]

Never used it, also unaware of its purpose 😰

[morozov]

Linking existing related issue #2700.

[morozov]

I spent some more time researching and figured out two more things:

1. Looks like the command relies on a non-documented PDO feature where it can execute multiple queries at once given the prepared statement emulation is enabled (source). Therefore, it doesn't work with non-PDO drivers (mysqli, oci8, sqlsrv produce an expected syntax error, ibm_db2 silently does nothing).

2. In its current state, the condition:

   dbal/lib/Doctrine/DBAL/Tools/Console/Command/ImportCommand.php

   Line 96 in df43437

   if ($conn instanceof \Doctrine\DBAL\Driver\PDOConnection) {

   always evaluates to false since $conn is a wrapper connection:
   

   dbal/lib/Doctrine/DBAL/Tools/Console/ConsoleRunner.php

   Lines 98 to 99 in df43437

   	// replace with the mechanism to retrieve DBAL connection in your app
   	$connection = getDBALConnection();

   , therefore, the offending code can at least be removed as dead.

This command cannot be fixed across platforms (see, #2700 (comment)), therefore, unless there's a reason to keep is as is, I'd remove it entirely. For the end-user, it can be almost transparently replaced with something like mysql < data.sql.

Any objections?

[Ocramius]

Fine by me 👍 Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/

…

On Tue, Aug 7, 2018 at 5:41 AM Sergei Morozov ***@***.***> wrote: I spent some more time researching and figured out two more things: 1. Looks like the command relies on a non-documented PDO feature where it can execute multiple queries at once given the prepared statement emulation is enabled (source <https://phpdelusions.net/pdo#multiquery>). Therefore, it doesn't work with non-PDO drivers (mysqli, oci8, sqlsrv produce an expected syntax error, ibm_db2 silently does nothing). 2. In its current state, the condition: https://github.com/doctrine/dbal/blob/df4343721d9981e58afd15eeaf18c885a2399766/lib/Doctrine/DBAL/Tools/Console/Command/ImportCommand.php#L96 always evaluates to false since $conn is a wrapper connection: https://github.com/doctrine/dbal/blob/df4343721d9981e58afd15eeaf18c885a2399766/lib/Doctrine/DBAL/Tools/Console/ConsoleRunner.php#L98-L99, therefore, the offending code can at least be removed as dead. Unless there's a reason to keep this code, I'd remove this command entirely. For the end-user, it can be almost transparently replaced with something like mysql < data.sql. Any objections? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3237 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakPcAch9pqPR1WGcqUbwkGtHmAQAeks5uOQxMgaJpZM4VtcKS> .

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.