Try out ProxyManager

[wouterj]

I started trying out some of the ideas in #110 and #113.

The idea of this change is that instead of a "fake" Proxy object, we return a proxied entity/model. This allows removing ->object() calls and removing some confusion when trying to pass the objects into typehinted functions. For this to work, the solution has to fulfill these promises:

• [x] The proxied entity/model has to support auto-refresh at least as good as the current Proxy
• [x] The AR helpers (save(), remove(), ...) have to be moved to Factory/ModelFactory
• [x] In 1.x, this new behavior should be opt-in per factory. This allows a smooth migration path, before removing the old factories in 2.0
• [ ] Reintroduce support for global auto refresh setting
• [ ] Document UPGRADE guide + deprecate the old Proxy

[wouterj]

Btw, I can't reproduce the Psalm errors locally :confused:

[wouterj]

Am I correct in that we can't add methods to the generated proxy?

Yes. Proxies, at least within the scope of the ProxyManager library, are meant to behave exactly like the real object (with the proxy thing being an implementation detail). The proxy manager has a couple proxy types: value holder (used here, allows you to call code before and after each real object method call); and lazy loading proxies (those you know from Doctrine, where the entity is only fetched from the db when you call a method).

Are you planning a way to enable globally via config?

I think it should be doable. I made the opt-in per factory at the moment, as that allows you to migrate one factory at the time. Also, it means that e.g. third party factories can already add support for it while your application's factories don't.

[wouterj]

To summarize our Slack conversation:

• [x] Move these proxy methods to normal functions: force_set, force_get, disable_autorefresh, enable_autorefresh, without_autorefresh
• [x] Move save and remove to methods on the Factory
• [x] Handling auto refresh while the kernel was rebooted is a must have

[wouterj]

I've updated the PR solving the biggest blocker: refreshing the model (value holder) of the proxy.

I'll try to find some time later this week/weekend to fix the 2 remaining (simple) tasks. After that, this should be ready :)

[kbond]

Btw, I can't reproduce the Psalm errors locally

https://github.com/laminas/laminas-code/issues/67 looks similar.

[wouterj]

PR updated once again with the helper functions and an updated README. As the proxy object now really behaves like the real object, I've removed most of the mentions of Proxy (it's mostly an implementation detail now).

While documenting, I discovered 2 remaining tasks that I've added to the PR description.

[kbond]

Awesome, thanks for continuing to work on this! Have you thought anymore a out how we're going to migrate users to this? It'd be nice if new installs auto used this and existing installs have to choose one way or another (like what we did in #131). The maker should be updated as well.

[wouterj]

Have you thought anymore a out how we're going to migrate users to this?

Currently, you would opt-in for the new proxy manager in each factory:

class PostFactory extends ModelFactory
{
    protected function initialize()
    {
        return $this->withProxyGenerator();

        // or, if you want to limit proxy to only some methods (supporting globs)
        //return $this->withProxyGenerator(['get*']);
    }
}

I suggest adding a deprecation notice in Factory#create() if auto refresh is true and the proxy generator was not enabled (i.e. the old proxies are in use). The make:factory maker should indeed be updated to use ->withProxyGenerator() by default. In Symfony's BC policy, this means throwing an exception if withProxyGenerator() is not called in Foundry 2 and removing the method in Foundry 3. A less strict, but imho OK, solution is to remove the method in Foundry 2 directly and always use the proxy generator if auto refresh is true.

Optionally, we can add a global option as you suggested. The advantage would be an easier migration when we remove the withProxyGenerator() method. The disadvantage is that this disallows updating one factory each time (when updating the docs, I realized it's quite some things you have to change). I'm happy to hear if you would like such a global option :)

[kbond]

I'm on board 100% with regards to ModelFactory's and giving the ability to upgrade on a factory-by-factory basis. With AnonymousFactory's it will be tricky without a global option. We certainly don't want users to have to call withProxyGenerator() for these.

Following Symfony's BC policy, we can't remove the current Proxy system entirely in 2.0?

[kbond]

What about hooking into the current auto_refresh_proxies config and making it an enum: new, old, disable (need help with better names :thinking:) and make using a boolean deprecated?

[wouterj]

Following Symfony's BC policy, we can't remove the current Proxy system entirely in 2.0?

Introducing a new system is a 3 step progress in Symfony BC:

1. In a minor version (x.y): Introduce the new system, add an option/method to opt in to the new system, and deprecate not doing the opt-in (i.e. trigger deprecation if withProxyManager() isn't called)
2. On the next major version ((x+1).0): switch the opt-in to opt-out and throw an exception if the new system isn't opt-in (i.e. withProxyManager() isn't called). You can safely remove the old system now.
3. On the next major version ((x+2).0): remove the opt-in.

If you remove the method directly in step (2), you require people to update their code to remove the opt-in. This is also why I think it's very strict and OK for smaller libraries to not do this (the method is a useless bit of code hanging around for the entirety of Foundry 2).

What about hooking into the current auto_refresh_proxies config and making it an enum: new, old, disable (need help with better names thinking) and make using a boolean deprecated?

Hmm, this is an interesting idea. This would allow you to change the "default" and change it on a factory basis using withProxyManager(bool $enabled = true). This is the best of 2 worlds: an app that is fully migrated wouldn't have withProxyManager() anywhere and everyone is free to update each factory individually.

I think adding a new boolean option would make the most sense ("new" doesn't mean much in Foundry 2). What about auto_refresh: true/false?

[kbond]

If you remove the method directly in step (2), you require people to update their code to remove the opt-in. This is also why I think it's very strict and OK for smaller libraries to not do this (the method is a useless bit of code hanging around for the entirety of Foundry 2).

Yeah, this does seem a bit strict. I'd like to remove Proxy (and related code) completely in 2.0. Do you agree? If so, withProxyManager wouldn't exist in 2.0, correct? I'd hate to add this to the maker, then deprecated and remove in 2.0. Or maybe I'm not fully understanding?

I think adding a new boolean option would make the most sense ("new" doesn't mean much in Foundry 2). What about auto_refresh: true/false?

So what would happen if auto_refresh: true and auto_refresh_proxies: true? Error?

[kbond]

Something that just occurred to me: public properties cannot be auto-refreshed. I don't know how common it is to use public properties on entities. I imagine with static analysis becoming more popular and read-only properties in 8.1 it will become more common.

[kbond]

I've been playing with an alternate implementation using Symfony 6.2's ProxyHelper (and a bit of hacking). Here's my POC: https://github.com/kbond/symfony-reproducer/tree/foundry-real-proxy

Here's the relevant commit and files:

• https://github.com/kbond/symfony-reproducer/blob/foundry-real-proxy/src/ProxyFactory.php (this is what foundry's Factory could look like)
• https://github.com/kbond/symfony-reproducer/blob/foundry-real-proxy/src/Proxy.php (the generated proxy implements this interface)
• https://github.com/kbond/symfony-reproducer/blob/foundry-real-proxy/src/IsProxy.php (the generated proxy uses this trait which implements the above interface)
• https://github.com/kbond/symfony-reproducer/blob/foundry-real-proxy/tests/RealProxyTest.php (the test that proves this works - everything is fully auto-completable in phpstorm)

Auto-refreshing doesn't work - maybe this is possible with some more tweaks. I'm also ok deprecating auto-refreshing and removing in 2.0. It is complex and has caused some confusion...

/cc @nikophil

[wouterj]

Auto-refreshing doesn't work - maybe this is possible with some more tweaks. I'm also ok deprecating auto-refreshing and removing in 2.0. It is complex and has caused some confusion...

In my honest opinion, if you plan on deprecating auto-refreshing I would advise to completely deprecate the proxy stuff. It's too complex and maintenance heavy to introduce a concept (AR) to entities that Symfony developers aren't really used to use.

I would then propose to just apply the changes of this PR related to moving the AR helpers to Factory: https://github.com/zenstruck/foundry/pull/127/files#diff-f1158fbec0bb446d87dbd5346fba683c58ccec1a98b1787511ec7786585e8c31

[kbond]

In my honest opinion, if you plan on deprecating auto-refreshing I would advise to completely deprecate the proxy stuff. It's too complex and maintenance heavy to introduce a concept (AR) to entities that Symfony developers aren't really used to use.

That's a fair point. Although, current users of foundry are used to the AR concept (assuming they're using these methods on the current Proxy object).

I'm not sure yet about deprecating auto-refreshing... When it works, it's quite nice.

[kbond]

My POC now includes auto-refreshing methods (not public properties):

$post->_enableAutoRefresh();

$post->getTitle(); // auto-refreshes before returning value
$post->body; // does not auto-refresh...