mrjgreen/db-sync

Please add missing test db SQL

dotnetCarpenter opened this issue · 9 comments

It's pretty hard to hack and run the tests with db-sync, when I don't have the test data, making it difficult to contribute.

Could you please commit a SQL dump with the dbsynctest tables?

I'm trying to fix an issue with a horrible db I have inherit where a lot of DATE fields has 0000-00-00 values, and I get the following error: SQLSTATE[22007]: Invalid datetime format: 1292 Incorrect date value: '0000-00-00' for column 'hascancelleddate' at row 1. I would like to fix it and create a PR but still know that the tests will pass.

If nothing else then I want to make sure that the error message is not cut off - I can not see the entire failing SQL statement. I've just begun to poke around the source but line 80 in sync is my starting point.

The table schemas are in here: https://github.com/mrjgreen/db-sync/blob/v3/tests/integration/FullSyncTest.php

Its not ideal - I'd like to move this out to an SQL file and utilise docker to help with the testing.

As for your issue, it seems like your target table most likely has a different sql-mode defined in the my.cnf, or is not defined and using the default sql-mode. Docs

Specifically NO_ZERO_IN_DATE and NO_ZERO_DATE modes. There are implicitly included in the STRICT_TRANS_TABLES or STRICT_ALL_TABLES mode, and in the TRADITIONAL mode.

So most likely, your my.cnf on the target is missing the sql-mode setting or has one of these:

sql-mode="NO_ZERO_IN_DATE,NO_ZERO_DATE"
sql-mode="STRICT_TRANS_TABLES"
sql-mode="STRICT_ALL_TABLES"
sql-mode="TRADITIONAL"
  1. Thanks! With some small changes to tests/integration/config.php, I'm able to run composer test now.

  2. I changed mysql.cnf but I still get the same error message.

[mysql]
sql-mode="ALLOW_INVALID_DATES"
  1. I've tracked the cut-off message culprit to be line 40 in ExceptionHandler.php. On line 104 in SyncCommand.php you set $this->output->setVerbosity(OutputInterface::VERBOSITY_DEBUG);. I was thinking that if the user uses the -v flag then we could lift $maxQueryLength to something bigger. In my case I need it to be well over 5000. But I'm trouble finding the instance creation of ExceptionHandler. What do you think?

closing because the original issue is resolved

Fixed number 2 with the answer here: http://stackoverflow.com/a/41781184/205696

The correct config in mysql.cnf is:

[mysql]

[mysqld]
sql-mode = "ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION,ALLOW_INVALID_DATES" 

Notice [mysqld], it will not work without it

As I see it, there is only 3 options to set the $maxQueryLength variable.

  1. Set visibility of $exceptionHandler; in Database\Connection to public
  2. Create a new class that inherit from Database\Connection and make that class available in SyncCommand and implement a setMaxQueryLength method that use $exceptionHandler->setMaxQueryLength according to verbosity level ($this->output->getVerbosity() === OutputInterface::VERBOSITY_DEBUG).
  3. Create a new class that implements ExceptionHandlerInterface. Change the ConnectionFactory make signature to accept an ExceptionHandler - all the way to createSingleConnection(array $config, $lazy) or use the $config object. Then in createSingleConnection we can add the new ExceptionHandler via $connection->setExceptionHandler.

The first option might violate the open/closed principle.

I think the second option is my favorite as it also allows for future customization without changing Database. For now, everything would be delegated to Database\Connection but with more granular control and possibility for db-sync extensions in the future.

@mrjgreen What do you think? Is it worth creating a PR or should I just monkey patch my local db-sync?
For now I've just did no 1. Which works fine for the moment. Just wondering what your thoughts are about this. If you accept, I could put in the time to create a proper PR.

I think a simple solution may be to refine your idea for option 2 and do something like this here, using the existing setters on the database connection, before returning the connection:

https://github.com/mrjgreen/db-sync/blob/v3/src/Command/SyncCommand.php#L219

$connection = (new ConnectionFactory())->make([
 //...
]);
$handler = new Database\ExceptionHandler();
$handler->setMaxQueryLength(200000); // Or set from an option
$connection->setExceptionHandler($handler);
return $connection;

You are right about option 3 though - the connection factory on the database package really should support a custom error handler.

I'd be happy to accept a PR for either option 3, or the revised option 2 from my comment above, if you feel like contributing! :)

Nice. I'll look into it in a few days, after I have finished a little CLI program (using both Database and DbSync).