zenstruck / foundry

A model factory library for creating expressive, auto-completable, on-demand dev/test fixtures with Symfony and Doctrine.
https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html
MIT License
607 stars 62 forks source link

ci: add sf 7.1 #622

Closed nikophil closed 1 week ago

nikophil commented 2 weeks ago

our proxy system does not work with symfony/var-exporter 7.1:

TypeError: Value of type null returned from ZenstruckFoundryTestsFixtureDocumentGenericDocumentProxy::__get() must be compatible with unset property ZenstruckFoundryTestsFixtureDocumentGenericDocumentProxy::$_autoRefresh of type bool

/home/runner/work/foundry/foundry/src/Persistence/IsProxy.php:118

or

TypeError: Cannot assign null to property AppDomainProgrammingProxy::$_autoRefresh of type bool

/app/opa-v4/vendor/zenstruck/foundry/src/Persistence/IsProxy.php:48

I don't understand why it fails now, because everything is fine in 7.0.8 and nothing big was release since then https://github.com/symfony/var-exporter/releases

Here is how a proxy class generated with ProxyGenerator looks like:

<?php

class ZenstruckFoundryTestsFixtureEntityGenericEntityProxy extends \Zenstruck\Foundry\Tests\Fixture\Entity\GenericEntity implements \Zenstruck\Foundry\Persistence\Proxy, \Symfony\Component\VarExporter\LazyObjectInterface
{
    use \Zenstruck\Foundry\Persistence\IsProxy, \Symfony\Component\VarExporter\LazyProxyTrait;

    private const LAZY_OBJECT_PROPERTY_SCOPES = [
        "\0".'Zenstruck\\Foundry\\Tests\\Fixture\\Model\\GenericModel'."\0".'date' => ['Zenstruck\\Foundry\\Tests\\Fixture\\Model\\GenericModel', 'date', null],
        "\0".'Zenstruck\\Foundry\\Tests\\Fixture\\Model\\GenericModel'."\0".'prop1' => ['Zenstruck\\Foundry\\Tests\\Fixture\\Model\\GenericModel', 'prop1', null],
        'date' => ['Zenstruck\\Foundry\\Tests\\Fixture\\Model\\GenericModel', 'date', null],
        'id' => [parent::class, 'id', null],
        'prop1' => ['Zenstruck\\Foundry\\Tests\\Fixture\\Model\\GenericModel', 'prop1', null],
    ];

    public function getProp1(): string
    {
        $this->_autoRefresh();

        if (isset($this->lazyObjectReal)) {
            return ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->getProp1(...\func_get_args());
        }

        return parent::getProp1(...\func_get_args());
    }

    public function setProp1(string $prop1): void
    {
        $this->_autoRefresh();

        if (isset($this->lazyObjectReal)) {
            ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->setProp1(...\func_get_args());
        } else {
            parent::setProp1(...\func_get_args());
        }
    }

    public function getDate(): ?\DateTimeImmutable
    {
        $this->_autoRefresh();

        if (isset($this->lazyObjectReal)) {
            return ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->getDate(...\func_get_args());
        }

        return parent::getDate(...\func_get_args());
    }

    public function setDate(?\DateTimeImmutable $date): void
    {
        $this->_autoRefresh();

        if (isset($this->lazyObjectReal)) {
            ($this->lazyObjectState->realInstance ??= ($this->lazyObjectState->initializer)())->setDate(...\func_get_args());
        } else {
            parent::setDate(...\func_get_args());
        }
    }
}

// Help opcache.preload discover always-needed symbols
class_exists(\Symfony\Component\VarExporter\Internal\Hydrator::class);
class_exists(\Symfony\Component\VarExporter\Internal\LazyObjectRegistry::class);
class_exists(\Symfony\Component\VarExporter\Internal\LazyObjectState::class);
nicolas-grekas commented 2 weeks ago

I see that autorefresh is called while the entity might not be initialized yet? Did you declare the _autoRefresh property as being "skipped" when constructing the lazy object?

nikophil commented 2 weeks ago

Hi @nicolas-grekas

I see that autorefresh is called while the entity might not be initialized yet?

hmmm indeed, maybe it would make sense to only call it if the object is initialized

Did you declare the _autoRefresh property as being "skipped" when constructing the lazy object?

I don't think :thinking: Not sure to see how to do that... Would that help in our problem? The property $_autoRefresh is coming from IsProxy trait which is used directly in the proxy class, I would have expected it to be initialized with its default property.

You can see here how the proxy is created.

nikophil commented 2 weeks ago

Something which is super strange (for me, at least :sweat_smile:) is that I have plenty of deprecations which say for instance:

Creation of dynamic property Zenstruck\Foundry\Tests\Fixture\Entity\Address\StandardAddress::$_autoRefresh is deprecated

meaning $this in the IsProxy trait context refers to StandardAddress and not to ZenstruckFoundryTestsFixtureEntityAddressStandardAddressProxy class

nicolas-grekas commented 2 weeks ago

Just follow my advice, the current use is not compliant with the lazy objects. Chose one way or the other but the current one is indeed broken ;)

nikophil commented 2 weeks ago

Hi @nicolas-grekas

I understand our implementation is broken, but I don't see how to fix it :sweat_smile:

how could I declare the property to be skipped, please? this sounds like the simplest solution

thanks!

nicolas-grekas commented 2 weeks ago

Check the signature of the factory method you're calling.

nikophil commented 2 weeks ago

You mean createLazyProxy()?

LazyProxyTrait::createLazyProxy() does not allow to do this :thinking:

public static function createLazyProxy(\Closure $initializer, ?object $instance = null): static

only LazyProxyTrait::createLazyGhost() does:

public static function createLazyGhost(\Closure $initializer, ?array $skippedProperties = null, ?object $instance = null): static