search

doctrine / dbal

Doctrine Database Abstraction Layer
https://www.doctrine-project.org/projects/dbal.html
MIT License
9.4k stars 1.33k forks source link

MySQL Decimal being Converted to Numeric #2802

Open d42ohpaz opened 6 years ago

d42ohpaz commented 6 years ago

This sounds like the issue with #1999.

I have a few entities set up as follows (relevant parts only):

class Account {
    /**
     * @ORM\Column(
     *     name         = "current_balance",
     *     type         = "decimal",
     *     precision    = 18,
     *     scale        = 2,
     *     nullable     = false,
     *     options      = {
     *         "default"    = "0.00"
     *     }
     * )
     */
    protected $currentBalance = 0.00;

    /**
     * @ORM\Column(
     *     name         = "starting_balance",
     *     type         = "decimal",
     *     precision    = 18,
     *     scale        = 2,
     *     nullable     = false,
     *     options      = {
     *         "default"    = "0.00"
     *     }
     * )
     */
    protected $startingBalance = 0.00;
}

class Transaction {
    /**
     * @ORM\Column(
     *     name         = "total_amount",
     *     type         = "decimal",
     *     precision    = 18,
     *     scale        = 2,
     *     nullable     = false,
     *     options      = {
     *         "default"    = "0.00",
     *         "unsigned"   = true
     *     }
     * )
     */
    protected $totalAmount = 0.0;
}

class Split {
    /**
     * @ORM\Column(
     *     name         = "amount",
     *     type         = "decimal",
     *     precision    = 18,
     *     scale        = 2,
     *     nullable     = false,
     *     options      = {
     *         "default"    = "0.00",
     *         "unsigned"   = true
     *     }
     * )
     */
    protected $amount;
}

When I go to run console doctrine:schema:update --dump-sql, I get the following output:

ubuntu@ubuntu-yakkety:/var/www$ console doctrine:schema:update --dump-sql
ALTER TABLE transaction_split CHANGE amount amount NUMERIC(18, 2) DEFAULT '0.00' NOT NULL;
ALTER TABLE transaction CHANGE total_amount total_amount NUMERIC(18, 2) DEFAULT '0.00' NOT NULL;

Here is my PHP version information:

PHP 7.1.7-1+ubuntu16.10.1+deb.sury.org+1 (cli) (built: Jul  7 2017 09:42:38) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.1.0, Copyright (c) 1998-2017 Zend Technologies
    with Xdebug v2.5.5, Copyright (c) 2002-2017, by Derick Rethans

And, here is my Symfony/Doctrine versions:

doctrine/annotations           v1.5.0  Docblock Annotations Parser
doctrine/cache                 v1.7.0  Caching library offering an object-oriented API for many cache backends
doctrine/collections           v1.5.0  Collections Abstraction library
doctrine/common                v2.7.3  Common Library for Doctrine projects
doctrine/dbal                  v2.5.13 Database Abstraction Layer
doctrine/doctrine-bundle       1.6.8   Symfony DoctrineBundle
doctrine/doctrine-cache-bundle 1.3.0   Symfony Bundle for Doctrine Cache
doctrine/inflector             v1.2.0  Common String Manipulations with regard to casing and singular/plural rules.
doctrine/instantiator          1.0.5   A small, lightweight utility to instantiate objects in PHP without invoking their constructors
doctrine/lexer                 v1.0.1  Base library for a lexer that can be used in Top-Down, Recursive Descent Parsers.
doctrine/orm                   v2.5.6  Object-Relational-Mapper for PHP
symfony/monolog-bundle         v3.1.0  Symfony MonologBundle
symfony/phpunit-bridge         v3.3.5  Symfony PHPUnit Bridge
symfony/polyfill-apcu          v1.4.0  Symfony polyfill backporting apcu_* functions to lower PHP versions
symfony/polyfill-intl-icu      v1.4.0  Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-mbstring      v1.4.0  Symfony polyfill for the Mbstring extension
symfony/polyfill-php56         v1.4.0  Symfony polyfill backporting some PHP 5.6+ features to lower PHP versions
symfony/polyfill-php70         v1.4.0  Symfony polyfill backporting some PHP 7.0+ features to lower PHP versions
symfony/polyfill-util          v1.4.0  Symfony uti lities for portability of PHP codes
symfony/swiftmailer-bundle     v3.0.3  Symfony SwiftmailerBundle
symfony/symfony                v3.3.5  The Symfony PHP framework

After poking around the source code, it appears that the Doctrine\DBAL\Types\DecimalType::getSQLDeclaration() is using Doctrine\DBAL\Platforms\AbstractPlatform::getDecimalTypeDeclarationSQL(), as it is not defined in Doctrine\DBAL\Platforms\MySqlPlatform. Looking at some of the other data type definitions, e.g., DATE, TIME, etc..., they each seem to be defined in Doctrine\DBAL\Platforms\MySqlPlatform.

While I can appreciate that in most RDBMs the NUMERIC(p, s) and DECIMAL(p, s) are interchangeable, it would be nice if DBAL would respect my choice in using the decimal type to mean DECIMAL(p, s) and not NUMERIC(p, s); if anything, a NumericType should be added for those who wish to use that type explicitly.

~Edit: It also seems that these DECIMAL/NUMERIC data types do not respect the options = { "unsigned" = true } flag.~ Nevermind. Caching issue.

Ocramius commented 6 years ago

NumericType should be added for those who wish to use that type explicitly.

I don't see a reason for that. A custom type is feasible at all time on your side, at the cost of losing the ability to have the schema comparator take over diffing for you.

From our PoV, this is just added work for no benefit of either party. As for those that really want to use DECIMAL(p, s), a manual migration is the quickest and most efficient solution.

d42ohpaz commented 6 years ago

I don't see a reason for that. A custom type is feasible at all time on your side, at the cost of losing the ability to have the schema comparator take over diffing for you.

I appreciate where you're coming from, but I would counter that losing the ability to have the schema comparator take over the diffing is a reason to have the specific type.

As for those that really want to use DECIMAL(p, s), a manual migration is the quickest and most efficient solution.

If I understand what you're saying here, you mean that I/we would always have to do console doctrine:schema:update --dump-sql, copy and paste that into whatever database client de jour, and then remember to modify NUMERIC to say DECIMAL?

With all due respect, that is counter-productive and counter-intuitive of what Doctrine is meant to offer its users. Not only do we use Doctrine to manage our data structures for us, but having to manually edit the schema is error prone (and in my opinion, respectfully, bad advice).

However, if the Doctrine\DBAL\Platforms\AbstractPlatform::getDecimalTypeDeclarationSQL() were to be modified to accept an optional second parameter (e.g., $type = 'numeric'), then you could (in very few lines of code) create Doctrine\DBAL\Types\NumericType that would extend Doctrine\DBAL\Types\DecimalType and only need to override getSQLDeclaration(). Then the DecimalType would be modified to pass in 'decimal' to its call on Doctrine\DBAL\Platforms\AbstractPlatform::getDecimalTypeDeclarationSQL(), while the NumericType would pass in nothing and rely on the default value from AbstractPlatform.

With all of that said, if the idea of the added work is still gnawing at you, I would be most happy to submit a PR for your review of the 4 sets of changes I described above.

Ocramius commented 6 years ago

If I understand what you're saying here, you mean that I/we would always have to do console doctrine:schema:update --dump-sql, copy and paste that into whatever database client de jour, and then remember to modify NUMERIC to say DECIMAL?

Yes, pretty much. Or else use a custom type declaration, which will not be recognized.

With all due respect, that is counter-productive and counter-intuitive of what Doctrine is meant to offer its users. Not only do we use Doctrine to manage our data structures for us, but having to manually edit the schema is error prone (and in my opinion, respectfully, bad advice).

It really is just about the Pareto principle: just use the DECIMAL type and you'll be fine. Need the NUMERIC type? You are outside the 80/20 split, and therefore it's unlikely worth going down the rabbit hole for both you and the DBAL project.

However, if the Doctrine\DBAL\Platforms\AbstractPlatform::getDecimalTypeDeclarationSQL() were to be modified to accept an optional second parameter (e.g., $type = 'numeric'), then you could (in very few lines of code) create Doctrine\DBAL\Types\NumericType that would extend Doctrine\DBAL\Types\DecimalType and only need to override getSQLDeclaration(). Then the DecimalType would be modified to pass in 'decimal' to its call on Doctrine\DBAL\Platforms\AbstractPlatform::getDecimalTypeDeclarationSQL(), while the NumericType would pass in nothing and rely on the default value from AbstractPlatform.

This is just for MySQL though, which means that the platform abstraction will no longer hold, as it contains details about the specific DB.

With all of that said, if the idea of the added work is still gnawing at you, I would be most happy to submit a PR for your review of the 4 sets of changes I described above.

I would suggest looking at how much effort it would be to just use NUMERIC, and what kind of pitfalls it causes on your side. It also is not strictly necessary for DBAL to manage your schema: I have dozens of projects where the complexity of the schema (even if by little) surpassed the complexity of the DBAL comparator and schema managers, and therefore I have to hand-author all the migration scripts anyway. A DBA would do that regardless of how perfect a DB migration tool is, because migrations can do scary stuff ;-)

Therefore, I discourage exploring this further unless the effort (consider also maintenance 10 years down the line) is lower than the suggestion of using the other alias type.

d42ohpaz commented 6 years ago

Need the NUMERIC type?

No, I never said I needed the numeric type. The defacto DBAL implementation already provides the numeric type. I need the decimal type (to actually be decimal).

You are outside the 80/20 split This is just for MySQL though, which means that the platform abstraction will no longer hold, as it contains details about the specific DB.

Each of the above databases support both DECIMAL and NUMERIC. What criteria are you using to define the 80/20 split? Seems to me that 100% of the DBAL platforms support both NUMERIC and DECIMAL.

If anything, this would make adding a NUMERIC type even easier. Add a AbstractPlatform::getNumericTypeDeclarationSQL(), extend DecimalType and override getSQLDeclaration() to use AbstractPlatform::getNumericTypeDeclarationSQL(). Done.

Therefore, I discourage exploring this further unless the effort (consider also maintenance 10 years down the line)

Given that AbstractPlatform::getDecimalTypeDeclarationSql() was added in September 2009, I hope you don't mean we have to wait another two years before we change make this change (kidding). But seriously, I would say the fact that this method hasn't had significant maintenance needed since its inception goes to show that the effort would be much lower and more useful to the community at large.

Also, the entire point of this ticket is that when using the schema comparator I get the wrong SQL statements. Using a custom type, as you've pointed out, would offer no benefit to me or others in the same situation.

d42ohpaz commented 6 years ago

PS. It's also worth noting that the SQL-99 Complete, Really standard define both DECIMAL and NUMERIC, so as beating dead horses goes, this is definitely not specific to MySQL.

Ocramius commented 6 years ago

Fair enough, that's the analysis I needed 😁

If you want to give it a shot, replicating the current DecimalType and logic around it, according to your findings, should be a few dozen lines of code and a bit more around testing.

Also, sorry for the decimal/numeric mixup: they read the same, work the same way, behave the same way, plus I'm multitasking 😅