DBAL-939: schema update doesnt detect boolean type correctly #2182

Open
doctrinebot opened this Issue Jul 16, 2014 · 2 comments

3 participants

@doctrinebot

Jira issue originally created by user rehfeldchris:

edited

Dbal for db2 doesn't seem to be able to differentiate between a smallint and a boolean field on db2. If I make an entity which has a boolean field, it will create a table using type smallint. If I later try to update the schema, it will detect the column in the table as a small int, not a boolean. So, it will produce sql update statements to change the type from smallint to smallint, which is obviously unnecessary. I understand db2 doesn't have a boolean type, but it would be nice if dbal realized that a smallint is essentially already a dbal boolean.

Additionally, the update statement is invalid syntax and fails, although this is probably a separate issue.

Example entity

<?php
/****
 * @ORM\Entity
 ****/
class Widget
{
    /*** @ORM\Id @ORM\Column(type="integer") @ORM\GeneratedValue ***/
    protected $id;

    /*** @ORM\Column(type="boolean", nullable=false) ***/
    private $isGoat;
}

orm:schema-tool:create produces:
CREATE TABLE Widget (id INTEGER GENERATED BY DEFAULT AS IDENTITY NOT NULL, isGoat SMALLINT NOT NULL, PRIMARY KEY(id));

If you then run orm:schema-tool:update, it will try to run:

ALTER TABLE WIDGET ALTER ISGOAT isGoat SMALLINT NOT NULL; CALL SYSPROC.ADMIN_CMD ('REORG TABLE WIDGET');

but no column alteration is obviosuly needed.

So in summary, smallint to boolean type mapping isn't realized, causing the update command to think it needs to change the type of the column.

@doctrinebot

Comment created by rehfeldchris:

I was thinking of giving this bug an attempt, but I want some advice on how to fix it properly from those who live and breathe this code base.

Additionally, I recently added a custom utcdatetime type , and now I'm having the same issue with that - doctrine reporting that I should alter all my date columns to modify them to a "TIMESTAMP(0)", even though they're already that type. So, I really think a proper solution would look at the actual mapping definitions being used to decide if a schema change should be suggested.

One idea I have is to modify Doctrine\DBAL\Schema\Comparator->diffColumn() so that it doesn't report this as a diff. However, I think this boolean to smallint mapping fact is platform dependent, so there would need to be some way for the Comparator to behave platform specific for this behavior. This sounds ugly to me.

Another idea would be to modify the specific platform objects themselves to take care of filtering out these frivolous differences in their getAlterTableSQL() methods. The platform object seems to know about type mappings, so maybe it would be appropriate to do something here.

I also see some event listeners that look like they might be able to be leveraged to pull this off here.

Any suggestions? Thanks.

@doctrinebot doctrinebot added the Bug label Dec 7, 2015
@beberlei beberlei was assigned by doctrinebot Dec 7, 2015
@deeky666 deeky666 assigned deeky666 and unassigned beberlei Jan 6, 2016
@deeky666 deeky666 added this to the 2.5.5 milestone Jan 6, 2016
@deeky666 deeky666 added the WIP label Jan 6, 2016
@deeky666 deeky666 added PR Created and removed WIP labels Jan 6, 2016
@deeky666 deeky666 added a commit to deeky666/dbal that referenced this issue Jan 6, 2016
@deeky666 deeky666 fix reverse engineering boolean type columns on DB2
- fixes platform's COMMENT ON statement support
- implements reverse engineering for column comments
- marks boolean column type commented for distinction

fixes #2182
02c14cf
@deeky666
Doctrine member

Patch provided in #2277

@deeky666 deeky666 added a commit to deeky666/dbal that referenced this issue Jan 8, 2016
@deeky666 deeky666 fix reverse engineering boolean type columns on DB2
- fixes platform's COMMENT ON statement support
- implements reverse engineering for column comments
- marks boolean column type commented for distinction

fixes #2182
dad3294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment