foundry icon indicating copy to clipboard operation
foundry copied to clipboard

[PHP8.4] Leverage PHP 8.4 lazy objects for proxy system

Open nikophil opened this issue 7 months ago • 8 comments

follow up for https://github.com/zenstruck/foundry/pull/893

Here is a list of things that need to be done to remove usage of ProxyHelper::generateLazyProxy()

  • [x] ~"undeprecate" auto_refresh_proxies configuration~
  • [x] ~add a method to PersistentObjectFactory to create objects that are not auto-refreshable~
  • [x] add helpers around proxies that will replace methods from IsProxy:
    • [x] delete
    • [x] _assertPersisted
    • [x] _assertNotPersisted
    • [x] ~_withoutAutoRefresh~
    • [x] ~_enableAutoRefresh~
    • [x] ~_disableAutoRefresh~ EDIT: i've not implemented _withoutAutoRefresh / _enableAutoRefresh / _disableAutoRefresh for the same reason we don't provide an auto_refresh_proxies option: all the "proxy quircks" should be gone now
  • [ ] Migration with Rector
  • [ ] impact documentation
  • [x] Use PHP 8.4 proxies for usage of factories in data providers https://github.com/zenstruck/foundry/pull/910

I think all those need to be shipped in the same release

nikophil avatar May 09 '25 06:05 nikophil

"undeprecate" auto_refresh_proxies configuration

This behaviour will be configurable? Would disabling just remove the events from the tracker? Making refresh_all() required? Or would it disable the system entirely?

add a method to PersistentObjectFactory to create objects that are not auto-refreshable

Why?

kbond avatar May 10 '25 15:05 kbond

"undeprecate" auto_refresh_proxies configuration

This behaviour will be configurable? Would disabling just remove the events from the tracker? Making refresh_all() required? Or would it disable the system entirely?

add a method to PersistentObjectFactory to create objects that are not auto-refreshable

Why?

haha, now that you ask the question, I'm not 100% sure 😅. Since this new proxy system has less quirks than the current one (no identity problem, no more error "Cannot auto refresh as there are unsaved changes"...) and since it does not "break" the type system anymore (in particular, compared to the "mixin" system), maybe we could just always create proxifed objects?

nikophil avatar May 12 '25 12:05 nikophil

add a method to PersistentObjectFactory to create objects that are not auto-refreshable

Ok, I understand now, this method would just trigger the created objects to not be tracked?

UserFactory::new()->untracked()->create();?

Thing we should wait until asked before adding. You can enable/disable globally to start.

kbond avatar May 12 '25 14:05 kbond

Ok, I understand now, this method would just trigger the created objects to not be tracked?

that's what I had in mind, but the more I think about out, the more I'm thinking it is not needed

Thing we should wait until asked before adding. You can enable/disable globally to start.

I think I will wait even to disable this globally 😅

nikophil avatar May 12 '25 14:05 nikophil

I think I will wait even to disable this globally

works for me.

"undeprecate" auto_refresh_proxies configuration

So we'd keep this deprecated and have nothing to do with this new proxy system?

kbond avatar May 12 '25 15:05 kbond

So we'd keep this deprecated and have nothing to do with this new proxy system?

yeah let's experiment while we're in 2.x, and see if users come with problems with this new proxy system. If this happens, we could implement a way to disable proxy

nikophil avatar May 18 '25 19:05 nikophil

todo rector rule :

  • [x] change PersistentProxyObjectFactory to PersistentObjectFactory
  • [x] replace calls to Proxy methods with helper equivalent:
    • [x] ->_save() to save()
    • [x] ->_refresh() to refresh()
    • [x] ->_delete() to delete()
    • [x] ->_get() to get()
    • [x] ->_set() to set()
    • [x] ->_assertPersisted() to assert_persisted()
    • [x] ->_assertNotPersisted() to assert_not_persisted()
  • [x] remove calls to Proxy methods that no longer exist:
    • [x] remove ->_enableAutoRefresh() calls
    • [x] remove ->_disableAutoRefresh() calls
    • [x] _withoutAutoRefresh() can be replaced with the content of its callback
  • [x] remove proxy() / unproxy() calls
  • [x] change phpdoc type hints where Proxy is used
  • [ ] ~change "real" Proxy type hints to the actual class~ this won't be done: too complex, and I don't think it is really done in the wild

@kbond any thoughts about this? do you think of something I'd forgotten?

I think this one can be complex, I don't know if I'll bother to do this

change phpdoc type hints where Proxy is used

nikophil avatar May 28 '25 09:05 nikophil

This all looks good - I can't think of anything else off hand.

kbond avatar May 29 '25 20:05 kbond