Undefined property error from proxy when EmbedOne field is not set

[Frederick888]

BC Break Report

Q	A
BC Break	yes
Version	2.0.0

Summary

When an EmbedOne field is not set in database, it now causes an Undefined property error.

Previous behavior

Previously in 1.2.x the field was simply left null.

Current behavior

An Undefined property is caused by generated proxy __get($name) method.

How to reproduce

For instance, I've got 2 models Lab and Address:

<?php

namespace App\Doctrine;

use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;

/**
 * @ODM\Document(collection="labs")
 */
class Lab extends BaseModel
{
    /**
     * @ODM\Id
     * @var string
     */
    public $_id;
    /**
     * @ODM\EmbedOne(targetDocument=Address::class)
     * @var Address
     */
    public $address;
}

<?php

namespace App\Doctrine;

use Doctrine\ODM\MongoDB\Mapping\Annotations as ODM;

/** @ODM\EmbeddedDocument */
class Address extends BaseModel
{
    /**
     * @ODM\Field(type="string")
     * @var string
     */
    public $country;
}

Now if I insert a new Lab document without touching the address field, it's also not set in the database as it used to be. However, when I retrieve the document, in the hydrator it's got

    public function hydrate(object $document, array $data, array $hints = array()): array
    {
        /** @EmbedOne */
        if (isset($data['address'])) {
            // omitted
        }
    }

...and in the proxy it's got

    /**
     * @var \Closure|null initializer responsible for generating the wrapped object
     */
    private $initializerc3072 = null;

    /**
     * @var bool tracks initialization status - true while the object is initializing
     */
    private $initializationTrackerce44e = false;

    /**
     * @var bool[] map of public properties of the parent class
     */
    private static $publicPropertiesda3cc = [
        'address' => true,
    ];

    public function __get($name)
    {
        $this->initializerc3072 && ! $this->initializationTrackerce44e && $this->callInitializer728ea('__get', array('name' => $name));

        if (isset(self::$publicPropertiesda3cc[$name])) {
            return $this->$name;  // BREAKS HERE
        }
        // omitted
    }

Since the address field is not set in the hydrator, when the proxy tries to get its value return $this->$name it causes an Undefined property error.

PS: Not sure whether it's relevant but BaseModel is

<?php

namespace App\Doctrine;

use Carbon\Carbon;
use MyCLabs\Enum\Enum;

class BaseModel
{
    protected $excludedFromArray = ['_id', 'lazyPropertiesDefaults'];
    private $preserveOriginalName = false;

    public function __construct(array $data = null)
    {
        if (!empty($data)) {
            foreach ($data as $key => $value) {
                $this->$key = $value;
            }
        }
    }

    public function __get($name)
    {
        $accessor = 'get_' . $this->snakeCase($name);
        if (method_exists($this, $accessor)) {
            return $this->$accessor();
        }
        return $this->$name ?? null;
    }

    public function __set($name, $value)
    {
        $mutator = 'set_' . $this->snakeCase($name);
        if (method_exists($this, $mutator)) {
            // an additional bool parameter ($toRemove = false) is provided
            $this->$mutator($value, false);
        } else {
            $this->$name = $value;
        }
    }

    public function __unset($name)
    {
        $this->__set($name, null);
    }

    public function __isset($name)
    {
        return $this->__get($name) !== null;
    }

    public function __clone()
    {
        foreach (get_object_vars($this) as $key => $value) {
            $this->$key = $this->cloneRecursively($value);
        }
    }

    /**
     * @return array
     */
    public function getExcludedFromArray(): array
    {
        return $this->excludedFromArray;
    }

    /**
     * @param array $excludedFromArray
     */
    public function setExcludedFromArray(array $excludedFromArray): void
    {
        $this->excludedFromArray = $excludedFromArray;
    }

    /**
     * @param mixed $item
     * @return mixed
     */
    private function cloneRecursively($item)
    {
        if (is_object($item)) {
            return clone $item;
        }
        if (is_array($item)) {
            return array_map([self::class, 'cloneRecursively'], $item);
        }
        return $item;
    }

    public function toArray(): array
    {
        $reflect = new \ReflectionClass($this);
        $result = [];
        foreach ($reflect->getProperties(\ReflectionProperty::IS_PUBLIC) as $key) {
            $key = $key->getName();
            if (strpos($key, '__') === 0
                || in_array($key, $this->getExcludedFromArray(), true)
                || method_exists($this, 'get_' . $this->snakeCase($key))) {
                continue;
            }
            $result[$key] = $this->toArrayRecursively($this->$key);
        }
        foreach ($reflect->getMethods() as $method) {
            $method = $method->getName();
            if (preg_match('/^get_(.+)$/', $method, $matches)) {
                $key = $matches[1];
                if (strpos($key, '__') === 0 || in_array($key, $this->getExcludedFromArray(), true)) {
                    continue;
                }
                $result[$key] = $this->toArrayRecursively($this->{$matches[0]}());
            }
        }
        return $result;
    }

    /**
     * @param mixed $item
     * @return mixed
     */
    private function toArrayRecursively($item)
    {
        if ($item instanceof Carbon) {
            return $item->format('c');
        }
        if ($item instanceof \DateTimeInterface) {
            return Carbon::instance($item)->format('c');
        }
        if ($item instanceof Enum) {
            return $item->getValue();
        }
        if ($item instanceof \IteratorAggregate) {
            $item = $item->getIterator();
        }
        if ($item instanceof \Iterator) {
            $item = iterator_to_array($item);
        }
        if (is_object($item) && method_exists($item, 'toArray')) {
            return $item->toArray();
        }
        if (is_array($item)) {
            return array_map([self::class, 'toArrayRecursively'], $item);
        }
        return $item;
    }

    protected function snakeCase($name): string
    {
        if ($this->preserveOriginalName) {
            return $name;
        }
        return strtolower(preg_replace('/[^A-Z0-9]+/i', '_', $name));
    }
}

[malarzm]

@Frederick888 could you please report an issue to https://github.com/Ocramius/ProxyManager as it seems to be a shortcoming in the proxying mechanism itself? For the time being you may consider using a getter method that should work just fine as it won't utilize magic __get under the hood.

[alcaeus]

@Frederick888 out of curiosity, does the error appear when changing property visibility to protected or private? Not saying you should change the code, but I remember there were some special things about public properties in proxies.

[Frederick888]

@malarzm

report an issue to https://github.com/Ocramius/ProxyManager

No problem. But honestly I've got no idea what OOP proxies are... What would be an appropriate title for the issue there?

you may consider using a getter method

You meant a getter in my own model? It's already got one in BaseModel:

    public function __get($name)
    {
        $accessor = 'get_' . $this->snakeCase($name);
        if (method_exists($this, $accessor)) {
            return $this->$accessor();
        }
        return $this->$name ?? null;
    }

@alcaeus

changing property visibility to protected or private

This time the undefined address error is gone but instead I got undefined excludedFromArray (the toArray filter I've got in BaseModel). I guess it's because the property is unset in the proxy?

    public static function staticProxyConstructor($initializer)
    {
        static $reflection;

        $reflection = $reflection ?? $reflection = new \ReflectionClass(__CLASS__);
        $instance = $reflection->newInstanceWithoutConstructor();

        unset($instance->address, $instance->excludedFromArray);

        \Closure::bind(function (\App\Doctrine\BaseModel $instance) {
            unset($instance->preserveOriginalName);
        }, $instance, 'App\\Doctrine\\BaseModel')->__invoke($instance);

        $instance->initializerc3072 = $initializer;

        return $instance;
    }

[malarzm]

@Frederick888 something like "Undefined property error for public property and __get" should do. Also you can link to this issue, but I'm sure @Ocramius will either know what to do or what questions to ask :)

[alcaeus]

Assigning a bug label for now, I hopefully may have time in the coming days to take a look at this as it's quite severe.

[malarzm]

Closing as #2082 was merged