Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MariaDB 10.2 initial support #2825

Merged
merged 45 commits into from Nov 19, 2017
Merged

Conversation

@belgattitude
Copy link
Contributor

@belgattitude belgattitude commented Aug 26, 2017

Initial support for MariaDB 10.2.7+.

Doctrine currently does not support MariaDB 10.2.7+. Schema creation/update is triggering infinite schema diffs

Updated: Title updated from 'MariaDB 10.2 support (will also fix version detection), version detection should be considered as new feature.

Features

  • 1. MariaDb version detection (strip '5.5.5-' prefix)

To circumvent a limitation in the replication protocol, MariaDB 10+
use a prefixed version with '5.5.5-' (for instance 5.5.5-Mariadb-10.0.8-xenial).
This cause issues with the current detection mechanism with both Mysqli and PDOMysql
dbal implementations (always detect 5.5.5). While mariadb strips the prefix
automatically in their libmariadb-client, most (if not all) of php installations
relies on mysqlnd, so the removal of the prefix 5.5.5- must be made
in userland (MysqliConnection.php and AbstractMySQLDriver.php).
See https://jira.mariadb.org/browse/MDEV-4088 for reference.

  • 2. MariaDb 10.2 support BLOB/TEXT defaults values (since 10.2.1).

Update: Removed from this P/R... will be made available in a subsequent P/R (due to architecture decision to be made - current doctrine implementation needs to be reworked)

Note that Mysql (as of 5.7.19) is not yet supporting defaults for BLOB and TEXT.
See https://mariadb.com/kb/en/library/blob-and-text-data-types

  • UPDATED 3. MariaDb 10.2 support JSON type.

Removed from P/R by @lcobucci, probably to reflect the fact that MariaDB alias JSON to MEDIUMTEXT

The JSON type have been introduced in 10.2.7 as an alias to LONGTEXT.
See https://mariadb.com/kb/en/library/json-data-type/
To ensure portability between MySQL/Mariadb, the best is to prefer TEXT type over JSON when defining schema (till a real JSON type appears in Maria).

  • 4. MariaDb 10.2 supports CURRENT_TIME, CURRENT_DATE as default values

Note that MySQL (as of 5.7.19) does not support default values CURRENT_TIME and
CURRENT_DATE as default values for TIME and DATE columns (CURRENT_TIMESTAMP
for DATETIME is supported).

  • 5. Default literal values must be escaped.

Update: Removed from this PR in favor of #2850... Current doctrine implementation does not handle escaping of defaults (backslashes...), adding it in this P/R should be done for all platforms at once and thus introduce more BC-risks (bugs...). Implementing separately looks safer and should help having this P/R sooner. Both P/R can be done independently.

While table comments are automatically quoted (MySQLPlatform::quoteStringLiteral()) the
default column values were not. Defaults values for string literals are now properly quoted.

  • 6. MariaDB 10.2.7 information changes schema for default values.

In order to allow expressions as default values and distinguish them
from literals, Mariadb now quotes default values in information_schema.column table.
This changes brings a lot of incompatibilities between the (Oracle-)Mysql/MariaDB platforms.

Instead of creating a specific SchemaManager for MariaDB, the solution taken in this P/R is to map
the introduced changes in the current MySQLSchemaManager (method: getMariaDb1027ColumnDefault()).

From MariaDB 10.2.7, information schema changes includes:

  • NULL is now quoted as 'NULL' (see exceptions in https://jira.mariadb.org/browse/MDEV-14053. Unquoted null is technically possible)
  • String defaults are quoted (To save the string 'NULL', set the default as ''NULL'').
  • CURRENT_TIMESTAMP, CURRENT_DATE, CURRENT_TIME defaults are silently changed to 'current_timestamp()', 'currdate()', 'currtime()'.
    To prevent schema diff, they are mapped back to their original values.
  • (UPDATED) Partial support for default literal escaping have been done. Currently
    limited on schema introspection. #2850 will bring the support for backslashes in defaults later on
    but the code is ready.

See https://mariadb.com/kb/en/library/information-schema-columns-table/ for reference and
https://jira.mariadb.org/browse/MDEV-13132, https://jira.mariadb.org/browse/MDEV-13341 for history.

Will fix:

  • #2817 (invalid detection of column default values)
  • doctrine/orm#6565 (schema update will alter all tables with columns_default='null')
  • 7. Support for MariaDB 10.2 reserved keywords.

  • 8. Added MariaDB 10.2 in travis (both pdo_mysql and mysqli)

Possible BC-breaks:

None found. Since the extraction of defaults escaping in #2850, there should be no impact on other platforms.

Portability concerns

  • Portability of CURRENT_TIME and CURRENT_DATE, mariadb 10.2 supports them, but no way to get back to mysql.

Issues that will be fixed by this P/R:

DBAL related:

  • #2817 (invalid detection of column default values)

ORM related:

  • doctrine/orm#6565 (schema update will alter all tables with columns_default='null')
  • doctrine/orm#6790 (Doctrine schema update command shows always few queries to be executed)

Fix (or might fix) the following issues :

Other related open P/R

  • #2850 (Escaping of default values)
@belgattitude belgattitude changed the title Fix for Mariadb 10.2.7 default column value and nulls (maraidb change). Fix for Mariadb 10.2.7 default column value and nulls (mariadb change). Aug 26, 2017
@@ -178,12 +178,19 @@ protected function _getPortableTableColumnDefinition($tableColumn)

$length = ((int) $length == 0) ? null : (int) $length;

$isNotNull = ($tableColumn['null'] !== 'YES');
$columnDefault = (isset($tableColumn['default'])) ? $tableColumn['default'] : null;
if ($columnDefault === 'NULL' && !$isNotNull) {

This comment has been minimized.

@morozov

morozov Aug 26, 2017
Member

This looks a bit weird. How does MariaDB 10.2.7 distinct between default NULL and 'NULL'?

This comment has been minimized.

@belgattitude

belgattitude Aug 26, 2017
Author Contributor

Yes I was wondering too... looks they tried to fix something else and got a regression. Possible they will revert this... in this case we could eventually check the server version, and apply the fix selectively for a while.

This comment has been minimized.

@auviagroup

auviagroup Aug 26, 2017

They are not going to revert it... they are planing to enforce it as a standard in all upcoming versions, see https://jira.mariadb.org/browse/MDEV-13341
Quoted from above link:

It was an intentional change...

We've tried to reduce the damage by doing this change only in 10.2 (not in 10.1 or 10.0 or 5.5) and by keeping old behavior of SHOW COLUMNS (because it's non-standard, so there're no rules for it). Applications can switch to SHOW COLUMNS easily. Or adapt to use the new behavior, because old one cannot describe default values unambiguously.

This comment has been minimized.

@belgattitude

belgattitude Aug 26, 2017
Author Contributor

Was wondering, but now I've tested and figured out there's no problem at all. To the question of @morozov , how to disambiguate ? actually very simple:

CREATE TABLE `my_table` ( 
    `column_1` VARCHAR(50) CHARACTER SET utf8 COLLATE utf8_general_ci NULL DEFAULT NULL , 
    `column_2` VARCHAR(50) CHARACTER SET utf8 COLLATE utf8_general_ci NULL DEFAULT 'NULL' 
) ENGINE = InnoDB;

If you read the information_schema, like

SELECT * FROM `COLUMNS` where TABLE_SCHEMA = 'mytest'

You'll see defaults are NULL and 'NULL'. So no problem.

This comment has been minimized.

@morozov

morozov Aug 27, 2017
Member

@belgattitude my question was about the information schema, not about the actual tables. What does it contain for the columns above?

'Type' => 'datetime',
'Null' => 'YES',
'Key' => '',
'Default' => 'NULL', // MariaDB 10.2.7 return 'NULL'

This comment has been minimized.

@morozov

morozov Aug 26, 2017
Member

It's better to have this as a functional test. It's an implementation detail of a given DB version, not really an expectation of the DBAL.

This comment has been minimized.

@belgattitude

belgattitude Aug 26, 2017
Author Contributor

Yep, but too new to the project to figure out quickly how the test suite runs. Just mocked as a first step.

Copy link

@auviagroup auviagroup left a comment

Tested solved the issue with mariadb 10.2.7... suggesting to change the comment in line 184 from "// for mariadb 10.2.7" to "// for mariadb 10.2+"

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Aug 27, 2017

Thanks @auviagroup , changed the comment to >= 10.2.7 instead. Don't know the policy of doctrine concerning code... maybe I'll remove.

@@ -178,12 +179,27 @@ protected function _getPortableTableColumnDefinition($tableColumn)

$length = ((int) $length == 0) ? null : (int) $length;


$isNotNull = (bool) ($tableColumn['null'] !== 'YES');

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

The comparison result is already boolean. I see it's copied from the existing code, but anyways.


// For MariaDB >= 10.2.7
if ($this->_platform instanceof MariaDb102Platform) {
$columnDefault = (!$isNotNull && $tableColumn['default'] === 'NULL') ? null : $tableColumn['default'];

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

Is it possible that the column is NOT NULL but the default is NULL? Why do we check !$isNotNull?

This comment has been minimized.

@belgattitude

belgattitude Aug 28, 2017
Author Contributor

You right, looks they fixed it:

SQL_MODE has been changed; in particular, NOT NULL fields with no default will no longer fall back to a dummy value for inserts which do not specify a value for that field.

(try col INT(10) NOT NULL DEFAULT NULL in previous versions, to be sure. MariaDb 10.2 is ok)

}
} else {
// Regular MySQL
$columnDefault = (isset($tableColumn['default'])) ? $tableColumn['default'] : null;

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

Is it possible that the 'default' key does not exist in the column definition?

This comment has been minimized.

@belgattitude

belgattitude Aug 28, 2017
Author Contributor

Nope, but it was originally like that. I guess it was made on purpose.
For me we can replace by $tableColumn['default'] === null. But I would not add this to the current P/R. Not sure about all mysql versions.

$table = new Table("text_null_default_value");
$table->addColumn('txt_def_null', 'string', ['notnull' => false, 'default' => null]);
$table->addColumn('txt_def_null_as_string', 'string', ['notnull' => false, 'default' => 'NULL']);
$table->addColumn('txt_def_null_as_quoted_string', 'string', ['notnull' => false, 'default' => "NULL"]);

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

How is this different from the field above?

This comment has been minimized.

@belgattitude

belgattitude Aug 28, 2017
Author Contributor

Completely reworked now, thanks

$table->addColumn('txt_def_null', 'string', ['notnull' => false, 'default' => null]);
$table->addColumn('txt_def_null_as_string', 'string', ['notnull' => false, 'default' => 'NULL']);
$table->addColumn('txt_def_null_as_quoted_string', 'string', ['notnull' => false, 'default' => "NULL"]);
$table->addColumn('txt_def_null_as_doublequoted_string', 'string', ['notnull' => false, 'default' => '"NULL"']);

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

I'd replace this with a single-quoted string since string defaults will be single-quoted in information_schema.

This comment has been minimized.

@belgattitude

belgattitude Aug 28, 2017
Author Contributor

Good to know, I was wondering... I'll fix


$majorVersion = $versionParts['major'];
$minorVersion = isset($versionParts['minor']) ? $versionParts['minor'] : 0;
$patchVersion = isset($versionParts['patch']) ? $versionParts['patch'] : null;

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

GIven the regexps above, how can 'minor' and 'patch' keys be not set?

This comment has been minimized.

@belgattitude

belgattitude Aug 28, 2017
Author Contributor

Nice catch, thanks


$version = $majorVersion . '.' . $minorVersion . '.' . $patchVersion;
// Will catch things like '10.2.8-MariaDB-1~lenny-log'
preg_match('/(?P<major>\d{1,2})\.(?P<minor>\d{1,2})\.(?P<patch>\d+)/', $version, $firstMatch);

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

Why restrict the number of digits to 2 in major and minor but not in the patch? \d looks reasonable to me in all cases.

// Will catch things like '10.2.8-MariaDB-1~lenny-log'
preg_match('/(?P<major>\d{1,2})\.(?P<minor>\d{1,2})\.(?P<patch>\d+)/', $version, $firstMatch);
// Will catch things like '5.5.5-10.2.8-MariaDB-10.2.8+maria~xenial-log'
preg_match('/\-(?P<major>10)\.(?P<minor>\d{1,2})\.(?P<patch>\d+)/', $version, $secondMatch);

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

Which of the '10.2.8' is this supposed to match? The one after the '5.5.5' or after 'MariaDB'? Why only the major version of 10 is considered?

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

Both expressions are almost the same with the exception of the leading dash. Could they be combined together?

This comment has been minimized.

@belgattitude

belgattitude Aug 28, 2017
Author Contributor

Maybe, but for now I would consider to create a separate class or method (will be needed for mysqli support in MysqliConnection)... Yes I'm not happy with detection, but wanted something to start. It's in the todo. Once a better detection is ready I'll need your feedback.

throw DBALException::invalidPlatformVersionSpecified(
$version,
'MariaDB-<major_version>.<minor_version>.<patch_version>' .
'or <major_version>.<minor_version>.<patch_version>-MariaDB'

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

How come the 'MariaDB' marker is part of the version string requirement in the error message but not in the regexp?

This comment has been minimized.

@belgattitude

belgattitude Aug 28, 2017
Author Contributor

Good catch, anyway I want to move those tests somewhere else (and at the same time support mysqli detection, see todo list)


$version = $majorVersion . '.' . $minorVersion . '.' . $patchVersion;
if (version_compare($version, '10.2.7', '>=')) {
return new MariaDb102Platform();

This comment has been minimized.

@morozov

morozov Aug 28, 2017
Member

Would it make sense to name the class MariaDb1027Platform? Otherwise, it would be confusing that a connection to MariaDB 10.2.6 doesn't use the MariaDb102Platform class.

This comment has been minimized.

@belgattitude

belgattitude Aug 28, 2017
Author Contributor

Mmmmh, seems too much for me. Fix was released in 10.2.7, the first stable (GA) was 10.2.6. Will be forgotten soon. I guess no distributions include the first stable, and 10.2.6 was too much buggy for people to not update anyway.

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Aug 28, 2017

Thanks a lot @morozov !!! I've updated the P/R with some of your remarks. Not yet 100% ready, but I'll see what's possible to improve tonight. See also my previous comment with the roadmap

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Aug 28, 2017

Hi @morozov , maybe you can point me to some direction. To be able to work with mysqli driver, I will have to changeMysqliConnection as well and make a detection more or less identical to the tests made in AbstractMysqlDriver::getServerVersion().

If you look in MysqliConnection:

    public function getServerVersion()
    {
        $majorVersion = floor($this->_conn->server_version / 10000);
        $minorVersion = floor(($this->_conn->server_version - $majorVersion * 10000) / 100);
        $patchVersion = floor($this->_conn->server_version - $majorVersion * 10000 - $minorVersion * 100);

        return $majorVersion . '.' . $minorVersion . '.' . $patchVersion;
    }

This won't work with things like 5.5.5-10.2.8-MariaDB-10.2.8+maria~xenial-log... It will return 5.5.5 instead of 10.2.8.

So I'm thinking to make something like:

    public function getServerVersion()
    {
        $serverInfo = $this->_conn->get_server_info();
        if (false !== stripos($serverInfo, 'mariadb')) { // optional, prevent one more method call if makes sense
            $version = MariaDbVersionStringParserOrBetterName::getVersion($serverInfo);
            if ($version !== null) {
                return $version;
            }
        }

        $majorVersion = floor($this->_conn->server_version / 10000);
        $minorVersion = floor(($this->_conn->server_version - $majorVersion * 10000) / 100);
        $patchVersion = floor($this->_conn->server_version - $majorVersion * 10000 - $minorVersion * 100);

        return $majorVersion . '.' . $minorVersion . '.' . $patchVersion;
    }

What would be a good name and namespace for MariaDbVersionStringParserOrBetterName, or can it be in platform (then it should be static) ?

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Aug 28, 2017

@morozov
Copy link
Member

@morozov morozov commented Aug 28, 2017

What would be a good name and namespace for MariaDbVersionStringParserOrBetterName, or can it be in platform (then it should be static)?

According to the comment, the client just needs to strip the '5.5.5-' prefix, so I don't see the need in a special parser. Maybe we can just unify the logic of parsing MySQL and MariaDB versions so that we always parse the string representation, not the integer (because it doesn't work with all supported platforms).

Something like:

$serverInfo = $this->_conn->get_server_info();

if (false !== stripos($serverInfo, 'mariadb')) {
    $serverInfo = str_replace('5.5.5-', '', $serverInfo);
}

// parse $serverInfo
@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Aug 28, 2017

LGTM, I'll make a P/R... just trying to setup mariadb 10.2 on Travis and I'll commit

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Aug 28, 2017

Will fix this bug too: #2819.

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Aug 29, 2017

@belgattitude belgattitude changed the title Fix for Mariadb 10.2.7 default column value and nulls (mariadb change). Fix mariadb 10.2.7 default column values (nulls too) and mariadb detection Aug 29, 2017
@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Sep 1, 2017

@Ocramius,

looks you're working crazily on doctrine ;) Feel free to contact me about this P/R, if you need change, infos... I'll try my best to respond quickly.

Keep up the great work and thanks a lot.

if ('5' === $majorVersion && '7' === $minorVersion && null === $patchVersion) {
$patchVersion = '9';
}
if (!preg_match('/^(mariadb-)?(?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+)/', strtolower($version), $versionParts)) {

This comment has been minimized.

@Ocramius

Ocramius Sep 2, 2017
Member

This entire block needs to be moved to a private method

This comment has been minimized.

@belgattitude

belgattitude Sep 2, 2017
Author Contributor

Nice, just added AbstractMySQLDriver::getMariaDbMysqlVersionNumber($version).

if (version_compare($version, '10.2.7', '>=')) {
return new MariaDb102Platform();
}

This comment has been minimized.

@Ocramius

Ocramius Sep 2, 2017
Member

Is this block not supposed to return here?

This comment has been minimized.

@belgattitude

belgattitude Sep 2, 2017
Author Contributor

Did not want to change too much from the original code, all that is less than 10.2.7 will fallback to the last statement getDatabasePlatform() which actually sets the `MysqlPlatform' by default. I was not very happy with this neither, but tried to mimic what was done for Mysql57. Wait a bit I'll commit some changes soon, it will be more clear.


$version = $majorVersion . '.' . $minorVersion . '.' . $patchVersion;
if (!preg_match('/^(?P<major>\d+)(?:\.(?P<minor>\d+)(?:\.(?P<patch>\d+))?)?/', $version, $versionParts)) {

This comment has been minimized.

@Ocramius

Ocramius Sep 2, 2017
Member

Again to be moved to a private method

This comment has been minimized.

@belgattitude

belgattitude Sep 2, 2017
Author Contributor

Nice, just added AbstractMySQLDriver::getOracleMysqlVersionNumber($version).

if (version_compare($version, '5.7.9', '>=')) {
return new MySQL57Platform();
if ('5' === $majorVersion && '7' === $minorVersion && null === $patchVersion) {
$patchVersion = '9';

This comment has been minimized.

@Ocramius

Ocramius Sep 2, 2017
Member

Don't overwrite an existing version, please: use a new variable.

This comment has been minimized.

@belgattitude

belgattitude Sep 2, 2017
Author Contributor

Will be.

);
}
$majorVersion = $versionParts['major'];
$minorVersion = isset($versionParts['minor']) ? $versionParts['minor'] : 0;

This comment has been minimized.

$this->_sm->dropAndCreateTable($table);

$onlineTable = $this->_sm->listTableDetails("test_column_defaults_with_table");
$this->assertNull($onlineTable->getColumn('col0')->getDefault());

This comment has been minimized.

@Ocramius

Ocramius Sep 2, 2017
Member

It's self::assertNull

This comment has been minimized.

@belgattitude

belgattitude Sep 2, 2017
Author Contributor

Nice, I never noticed assertNull, assertEquals... were static. I fixed. Thx

/**
* For testing MariaDB 10.2+ / MySQL differences
*/
public function testColumnDefaultsWithCreate() {

This comment has been minimized.

@Ocramius

Ocramius Sep 2, 2017
Member

: void. Opening brace on next line.

}

/**
* For testing MariaDB 10.2+ / MySQL differences

This comment has been minimized.

@Ocramius

Ocramius Sep 2, 2017
Member

Put this description in the method name instead

This comment has been minimized.

@belgattitude

belgattitude Sep 2, 2017
Author Contributor

Changed to testColumnDefaultsWithCreateAreValidForMySqlAndMariaDb102

This comment has been minimized.

@belgattitude

belgattitude Sep 2, 2017
Author Contributor

Afterwards thinking, this test is newly introduced when working on MariaDB. Actually it's valid for both anyway. I'll cleanup the description completely. But good to know about method naming

$this->assertEquals(0, $t->getColumn('col9')->getDefault());
$this->assertEquals('He"ll"o world', $t->getColumn('col10')->getDefault());

// @todo mariadb returns 'current_timestamp()'

This comment has been minimized.

@Ocramius

Ocramius Sep 2, 2017
Member

What about this? Blocker for merging?

This comment has been minimized.

@belgattitude

belgattitude Sep 2, 2017
Author Contributor

mmm, let me one day more to see what I can do... From what I've seen it's not easy... The two syntaxes are accepted by mysql/mariadb. The only change is that with Mariadb, whatever syntax you used to create the table, it will always return current_timestamp() (don't remember where I read it, but from what I tested it's working like that).

@@ -817,6 +817,7 @@ public function testChangeColumnsTypeWithDefaultValue()

$columns = $this->_sm->listTableColumns($tableName);


This comment has been minimized.

@Ocramius

Ocramius Sep 2, 2017
Member

Revert

@belgattitude belgattitude reopened this Sep 2, 2017
@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Sep 2, 2017

@Ocramius

I tried to reflect your advices (at least as far as I understood them ;). Note that I had to update the .travis.yml file in order to test mariadb-10.2.8. Check if you like it that way. Note also that since your feedbacks I've updated the mariadb reserved keywords from the latest doc. Don't be surprised

Regarding the 'current_timestamp()' issue, I'll try to find a solution in a couple of days. As this behaviour was not yet tested, I fear to introduce BC with mysql. I'll let you know the possible risks (I guess I will need to alias current_timestamp and current_timestamp() to be sure no-one gets a schema update with doctrine/orm. It can make the code a bit unsexy).

It took me a while to get into the project conventions and CI setup, hope I'll be less messy next time.

Thanks for your feedbacks and the time you spend on this project. Fantastic design :)

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Sep 4, 2017

@Ocramius,

As I'm leaving for a short 6 days break on wednesday 6th, I can possibly fix some things tomorrow... or when I come back.

Regarding current_timestamp() issue, the current patch does not break the current behaviour (no regression). The current situtation may be summarized as:

If you're using MariaDB 10.2 and you're setting current_timestamp as default value instead of current_timestamp(), a doctrine schema update will be triggerred because 10.2 always stores current_timestamp() in information_schema whatever value you sets. As a workaround, always write current_timestamp() as default value.

As this patch fixes a lot of others too (travis, proper mariadb detection, null and default quoted values in information_schema), you may want to merge it sooner. Feel free to do, I can also make a separate p/r for current_timestamp when I come back (around the 11th).

All the best

@tristanbes
Copy link
Contributor

@tristanbes tristanbes commented Nov 21, 2017

yes, big thanks @morozov @belgattitude @lcobucci

@belgattitude belgattitude mentioned this pull request Nov 21, 2017
0 of 4 tasks complete
@ghost
Copy link

@ghost ghost commented Nov 21, 2017

@geeksareforlife What did you do to get your migrations to run, per this bug?

@geeksareforlife
Copy link

@geeksareforlife geeksareforlife commented Nov 21, 2017

@mpash I never had an issue with the migrations running, just with the migration creation when the schema was compared to the database.

So at the moment I am just removing all the alter table statements that are generated, but that obviously makes an entry point for errors - I have had at least two occasions where I removed too many statements and had to go back!

@tristanbes
Copy link
Contributor

@tristanbes tristanbes commented Nov 29, 2017

so, @belgattitude when do you think this patch can be released and tagged ? :)

@neufeind
Copy link

@neufeind neufeind commented Dec 5, 2017

TYPO3 is also looking forward to a new release so we can update the dependencies to have a release with this fix. Issue at TYPO3 referencing this: https://forge.typo3.org/issues/82023

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Dec 5, 2017

@tristanbes

when do you think this patch can be released and tagged ? :)

No idea. But in the meantime can you check everything is working for you ?

FYI, I had a report about some issues with dev-master on doctrine/orm#6565 (look at the end of the thread). But finally it appears to be something unrelated.

Anyway, always good to test. Just try a schema creation/update with your project.

"require": {
        //...
        "doctrine/dbal": "2.7.x-dev as 2.6.10",
        //...
}

And let me know.

@DamienHarper
Copy link

@DamienHarper DamienHarper commented Dec 7, 2017

@belgattitude @lcobucci : this release will be really appreciated by MariaDb users!

@tristanbes
Copy link
Contributor

@tristanbes tristanbes commented Dec 7, 2017

I can confirm @belgattitude that it fixes my problem.

Once the big migration has been generated and executed (a lot of ALTER TABLE xxx CHANGE yyy_id yyy_id INT DEFAULT NULL, CHANGE zzz_id zzz_id INT DEFAULT NULL) it does not keep trying to generate the same difffs

@DamienHarper
Copy link

@DamienHarper DamienHarper commented Dec 7, 2017

@belgattitude @lcobucci @tristanbes I also confirm diffs are OK with these fixes

@tristanbes
Copy link
Contributor

@tristanbes tristanbes commented Dec 8, 2017

Well, @belgattitude it seems that it's not entirely fixed.

  1. changing a column name
  2. generating diffs -> it will output like 50 changes instead of one.

The only issue it seems to be fixed is that without touching the schema, the migrations are not longer generated when without this patch, it constantly generates diffs

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Dec 9, 2017

hey @tristan, I cannot test right now cause I'm traveling for about a month without laptop. I'm not totally sure to understand your problem, if there is you'll have to wait till 27dec or try a pr.

@tristanbes
Copy link
Contributor

@tristanbes tristanbes commented Dec 9, 2017

Let me rephrase it @belgattitude:
Before this patch:

  1. running doctrine:schema:upgrade --force ran the same queries always and always and always.
  2. changing 1 thing on the schema (let's say add a new $name field on an entity generates a migration diff with tons of changes ALTER TABLE xxx CHANGE yyy_id yyy_id INT DEFAULT NULL, CHANGE zzz_id zzz_id INT DEFAULT NULL

AFTER this patch

  1. running doctrine:schema:upgrade --force no longers runs the same queries again and again
  2. changing 1 thing on the schema (let's say add a new $name field on an entity generates a migration diff with tons of changes (in my case generates 67 diff statements instead of 1 in the form of: ALTER TABLE xxx CHANGE yyy_id yyy_id INT DEFAULT NULL, CHANGE zzz_id zzz_id INT DEFAULT NULL)

So 2/ is not fixed, and is really a pain in the *** when generating diff for your application
I hope it's more clear. Unfortunatly, doctrine at this low level is beyond my skills for a PR.

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Dec 10, 2017

@tristanbes, weird. I remember having done that without problem. but not sure.

/cc @Bukashk0zzz because he might help to test. I've done some debugging with him recently. doctrine/orm#6565.

others might help too.it might be something else. testing dev-master includes more than my patch. could be nice to look deeper. at least see if it does not happen with mysql too.

@Bukashk0zzz
Copy link

@Bukashk0zzz Bukashk0zzz commented Dec 11, 2017

@tristanbes @belgattitude

Made test for the case that @tristanbes discarded and all works ok no problem detected.

Code: Bukashk0zzz/dbal-test#3

Test:
https://travis-ci.org/Bukashk0zzz/dbal-test/builds/314640238

@tristanbes
Copy link
Contributor

@tristanbes tristanbes commented Dec 12, 2017

@Bukashk0zzz : Mhhh, You have tested it against MariaDB 10.3. I'm having this issue for MariaDB 10.2. ("mariadb": "10.3" on your Travis configuration)

@Bukashk0zzz
Copy link

@Bukashk0zzz Bukashk0zzz commented Dec 13, 2017

@tristanbes No problem. Update to use MariaDB 10.2
Still all ok.

https://travis-ci.org/Bukashk0zzz/dbal-test/jobs/315753290

@tristanbes
Copy link
Contributor

@tristanbes tristanbes commented Dec 13, 2017

I found what causes the problem. When specifying the server_version this patch has NO EFFECT. It generates the same migrations over and over again.

doctrine:
  dbal: 
    server_version: 10.2

As soon as I remove server_version parameter, it no longer generates the wrong diff.

@belgattitude
Copy link
Contributor Author

@belgattitude belgattitude commented Dec 14, 2017

that makes sense. if you want to use server_version, you should set 10.2.7 instead of 10.2. the schema change appeared after 10.2.6. this will be confusing, but autodetection works.

@tristanbes
Copy link
Contributor

@tristanbes tristanbes commented Jan 3, 2018

Hello :-)
Sooooo, still no doctrine/dbal 2.7 so far ? I don't like to rely on dev-master for production :(

@Ocramius
Copy link
Member

@Ocramius Ocramius commented Jan 3, 2018

@tristanbes you guessed it right: no 2.7 yet ;-)

It will arrive, we're just not focused on it at the moment.

@jeffreymb
Copy link

@jeffreymb jeffreymb commented Jan 3, 2018

I get the feeling that there would be multiple people that would be really happy to see this merged... Is there any way we can encourage this to receive some focus? 😄

@Ocramius
Copy link
Member

@Ocramius Ocramius commented Jan 3, 2018

Not really, no.

Edit: to clarify (sorry, I'm just multitasking like crazy, didn't want to come off as rude), we're not working on pushing DBAL 2.7 out the door for now, but rather focused on stabelizing the ORM 3.0 development branch.

@neufeind
Copy link

@neufeind neufeind commented Jan 5, 2018

Could we get this into the 2.6-branch maybe? Another project currently using DBAL 2.6.x would be happy to have at least initial MariaDB 10.2-support.

@Ocramius
Copy link
Member

@Ocramius Ocramius commented Jan 5, 2018

PSA: Additions are ONLY EVER for minor/major releases. Stop asking.

@doctrine doctrine locked as resolved and limited conversation to collaborators Jan 5, 2018
@Ocramius

This comment has been minimized.

Copy link
Member

@Ocramius Ocramius commented on 4f876ef Jan 6, 2018

@Majkl578 tbh, mentioning is fine, but systems like bitbucket, as well as pushing to non-forks, can lead to a massive amount of noise.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.