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

Ignore non-nullable typed property for Embeddable when hydrating #8015

Conversation

sneakyvv
Copy link

@sneakyvv sneakyvv commented Feb 6, 2020

This is the PR requested for issue #8014.

Following the explanation in the bug report, when you have a null value in the database (which is valid because the parent property containing the Embeddable can be null, i.e. all the properties o f the Embeddable are null in the database), but the typed property is required (does not allow null), the hydrator should skip setting that null value to the typed property.

@sneakyvv sneakyvv force-pushed the 8014_ignore_null_values_in_embeddable_hydrate branch 4 times, most recently from ddfd379 to dbaaf37 Compare February 12, 2020 07:44
@sneakyvv sneakyvv force-pushed the 8014_ignore_null_values_in_embeddable_hydrate branch from dbaaf37 to dbac210 Compare February 12, 2020 07:59
@SenseException
Copy link
Member

@doctrine/doctrinecore Should this PR target the 2.7 branch?

@alcaeus
Copy link
Member

alcaeus commented Feb 12, 2020

Since 2.6 is no longer maintained, this indeed needs to target 2.7.

@sneakyvv sneakyvv changed the base branch from 2.6 to 2.7 February 12, 2020 15:58
@sneakyvv
Copy link
Author

target updated

PS: the CONTRIBUTING.md needs updating :-)

I am submitting a bugfix for a stable release
Your PR should target the lowest active stable branch (2.6).

@SenseException
Copy link
Member

SenseException commented Feb 12, 2020

target updated

PS: the CONTRIBUTING.md needs updating :-)

Thank you. I guess we could change the link to the documentation page where the supported versions are listed and remove versions from the CONTRIBUTING.md.

@beberlei
Copy link
Member

This is the wrong bugfix. The problem is that $foo->bar is created, even though its saved as NULL.

If you add the following to line 36, it already fails:

$this->assertNull($foo->bar);

The fix should be that this is already NULL. But thats actually hard to detect, because if all embeddable properities are null, should the embeddable be null? or is the embeddable present?

@beberlei beberlei closed this Feb 13, 2020
@beberlei
Copy link
Member

Maybe we can even use this, but its actually necessary to check this before line 89:

if ($value === null) {
    return
}

@sneakyvv
Copy link
Author

@beberlei I totally agree, and think the embeddable should be null, if all embeddable properties are null. But that's not the current behaviour. The current behaviour it to have the embeddable present (with all null values).
That's what I said in the bug report (#8014)

PS: Kinda related to #4568, because when all properties of the Embeddable are null, the parent property should also be null (which I fix now post-hydrating), but that occurs even later obviously, after all properties of the Embeddable are set.

Since #4568 ended rather badly... I decided to fix that myself like this with a PostLoad-listener:

<?php

namespace App\Infrastructure\Persistence\Doctrine;

use Doctrine\ORM\Event\LifecycleEventArgs;
use Doctrine\ORM\Mapping\ClassMetadata;
use Doctrine\ORM\Mapping\ReflectionEmbeddedProperty;
use Doctrine\Persistence\ObjectManager;

class NullableEmbeddableFixer
{
    private ObjectManager $objectManager;

    public function postLoad(LifecycleEventArgs $eventArgs)
    {
        $this->objectManager = $eventArgs->getObjectManager();

        $this->fixNullEmbeddables($eventArgs->getObject());
    }

    private function fixNullEmbeddables($object, \ReflectionProperty $parentProperty = null, $parentObject = null)
    {
        $hasNonNullProperties = false;
        /** @var ClassMetadata $embeddedMetadata */
        $embeddedMetadata = $this->objectManager->getClassMetadata(get_class($object));
        $embeddedClasses = $embeddedMetadata->embeddedClasses;
        /** @var \ReflectionProperty $property */
        foreach ($embeddedMetadata->getReflectionProperties() as $property) {
            if ($property instanceof ReflectionEmbeddedProperty) {
                continue;
            }
            if (array_key_exists($property->getName(), $embeddedClasses)) {
                $this->fixNullEmbeddables($property->getValue($object), $property, $object);
            }

            if (null !== $property->getValue($object)) {
                $hasNonNullProperties = true;
            }
        }

        if (!$hasNonNullProperties) {
            $parentProperty->setValue($parentObject, null);
        }
    }
}

I'm not familiar enough with Doctrine's inner workings, and how the same might be achieved in an other way. So if you see a solution, and I might be of any help, let me know!

@beberlei
Copy link
Member

@sneakyvv see #8022 please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants