Checks key exists rather than isset

[garoevans]

If the default value is set to null, isset will return false even though the key is actually there for a reason.

[doctrinebot]

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3425

We use Jira to track the state of pull requests and the versions they got
included in.

[garoevans]

No tests around this at the moment, wanted to push the change and get some feedback.

[deeky666]

What's the benfit of this? IMO there is no difference here between an option that contains the value null and an option that simply is not set. Needs a failing test case in order to get merged otherwise I simply don't get the problem. A null value for the options comment, unsigned, fixed and default has no special meaning. Am I missing something here?

[Ocramius]

Same thoughts as @deeky666. If there is a property that needs to allow null, then please add a test for it and it shall be merged without question.

[garoevans]

I have default set to NULL using annotations and my generated create schema includes DEFAULT NULL as I wanted.

However, when I run orm:schema-tool:update it tries to update the column as it thinks the default has changed. It hasn't changed, it just gets lost using isset where array_key_exists catches it.

If this still doesn't make sense give me some time and I will attempt to write a failing test around this.

[Ocramius]

@garoevans nothing gets merged without tests anyway (unless there isn't a way to test it) :-)

[garoevans]

Hopefully the test explains my problem?

[Ocramius]

Test seems good :-)

[garoevans]

@Ocramius do I need to do anything more with this now, or will it get merged in at some point?

[Ocramius]

@garoevans merged, thanks!

master: 1cc42d6
2.4: 39f2f0e

[deeky666]

What's the purpose of explicitly defining default with null? This is the default value for this property anyways and if you leave it it will be the same.

[garoevans]

For my specific use case, trying to come over to doctrine using an existing schema. The productions databases are fairly large and not planning on being changed any time soon. To use the schema tool the entities need to be handle a specifically declared null default to not force an alter statement regardless of change.

Also, regardless of the use case, this is "correct" where as the code previously had a bug, I think that is fair to say?

[deeky666]

@garoevans I can live with the change but your use case seems to be dealing with another issue. You want to make sure no useless alter table statements are created but even without this patch, a mapping that does not explicitly define a default should not trigger alter table statements if the database schema als uses null as default. This sounds more like an issue with the DBAL comparator then, rather than a mapping issue.
To um up: It should not be required to define options={"default":NULL} just to come around useless alter table statements, get what I mean?

[garoevans]

I'm not 100% I follow yet...

`some_field` varchar(60) COLLATE utf8_unicode_ci DEFAULT NULL

This is the current state of a text field for one of our tables. If we don't explicitly set '"default":NULL' in the annotations then the generated SQL is;

`some_field` varchar(60) COLLATE utf8_unicode_ci

Adding the default caused the issue I was experiencing that this patch fixes.

I think this is the correct and as expected in one way or another, I don't see how this is a problem with the comparator? You could argue that the comparator should know, but that may be giving it too much intelligence?

To jump onto the other side of the fence I'd be happy to take a look at it from the comparator side if you think there is still work to do?

[deeky666]

Okay I'll try to explain a bit more in detail.
If you instantiate a Doctrine\DBAL\Schema\Column object like this

$type = \Doctrine\DBAL\Types\Type::getType('string');

$column = new \Doctrine\DBAL\Schema\Column('some_field', $type);
$column->getDefault(); // NULL

$column = new \Doctrine\DBAL\Schema\Column('some_field', $type, array('default' => null));
$column->getDefault(); // NULL

// see: https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Schema/Column.php#L71

both columns are equal because \Doctrine\DBAL\Schema\Column::$default is null by default. So when the ORM schema tool builds columns from your mappings, it does not matter whether you define options={"default":NULL} or leave it.
When the existing schema is introspected by Doctrine DBAL, a column with a null default will be created equally to the above. Therefore the comparator should not detect schema changes on doctrine:schema:update and therefore should not ouput any SQL. But in your case it does so I assume there is a problem in DBAL with the comparator.
Can you maybe check your use case using DBAL 2.5 as I think to remember that we have fixed some things about default value comparison in that branch.

[deeky666]

Also please not this line. That makes it even more confusing why your generated SQL does not contain the DEFAULT NULL clause as you mapped a nullable column.

[garoevans]

I see what you're saying. I will put this at the bottom of the list, but will take a look into this, sounds interesting and off the top of my head I'm not sure how this is going wrong.