doctrine / DoctrineModule

Doctrine Module for Laminas
http://www.doctrine-project.org/projects/doctrine-module.html
MIT License
398 stars 269 forks source link

handleTypeConversions method not work correctly for nullable fields #649

Closed hkulekci closed 6 years ago

hkulekci commented 6 years ago

I am trying to hydrate an object with a nullable string field.

    /**
     * @ORM\Column(name="some_field_name", type="string", length=255, nullable=true)
     * @var ?string
     */
    protected $field;

And I am using the hydrate method of DoctrineObject with a null value. The result is an empty string. The problem is related to handleTypeConversions changes with #626 . This method does not work correctly for nullable values. I checked issues but cannot find the similar one. Sorry, if I missed it.

    $this->handleTypeConversions('', 'string');
    $this->handleTypeConversions(null, 'string');

Both of the calling return empty string.

hkulekci commented 6 years ago

ping @TomHAnderson

TomHAnderson commented 6 years ago

I don't like the way handleTypeConversions works at all. I've started another PR to correct it https://github.com/doctrine/DoctrineModule/pull/647 but that is dependent on changing the Doctrine DBAL and that's under debate: https://github.com/doctrine/dbal/pull/3291

In this very recent PR we move hydrator strategies before type conversion: https://github.com/doctrine/DoctrineModule/pull/646/files

So that means there's not a method to explicitly return a null through a hydrator strategy because the value is then munged by the handleTypeConversion script to the incorrect empty string.

While I don't expect to solve all the issues I've listed here I think a valid fix is to modify the DoctrineObject::handleTypeConversions to correctly handle null values.

hkulekci commented 6 years ago

We need an update for associated fields. If we have an associated field, that is nullable, isNullable method throws a MappingException. Because this method is checking only field mappings without checking associated fields. @vincequeiroz

hkulekci commented 6 years ago

Somebody try to solve the issue with another method instead of isNullable on Doctrine side, but it had closed.

https://github.com/doctrine/doctrine2/pull/6109/files

Also, have some issues about this :

https://github.com/doctrine/doctrine2/issues/7133 https://github.com/doctrine/doctrine2/issues/2668

Even, we create a pr for doctrine it takes time to merge. I have a solution to this problem. The way is a bit dirty, but I can handle. I will create a PR for the doctrine side. After merging doctrine pull request, we can change the code again.

hkulekci commented 6 years ago

fixed with #655