[1.x] fix using stories with simple objects #580
Conversation
a wip to fix using stories with objects that can't be persisted This is intended to be legacy code that shouldn't be merged / released in 2.x. fixes: zenstruck#573
| @@ -59,6 +59,8 @@ class Factory | |||
| /** @var callable[] */ | |||
| private array $afterPersist = []; | |||
|
|
|||
| private bool $shouldPersist = true; | |||
There was a problem hiding this comment.
wasn't it possible to directly use $this->persist?
There was a problem hiding this comment.
I want to say no - but I can't remember why we couldn't use $this->persist. When I get some bandwidth this week, i'll dive back into this PR and see if we can't tidy things up..
Note to self:
run step debugger through
Line 175 in b66ed4c
$this->persist was a no-go
There was a problem hiding this comment.
don't bother with debugging or what, I think it will be okay to merge as-is. First, because as you said, it's a bug fix. And secondly, it kinda OK to not have a super perfect code in 1.x which in few months will be legacy
There was a problem hiding this comment.
I've tested your PR and used Factory::$persist instead of the new property Factory::$shouldPersist` and all tests are green 🤔
| @@ -50,6 +50,8 @@ final class Proxy implements \Stringable, ProxyBase | |||
| public function __construct( | |||
| /** @param TProxiedObject $object */ | |||
| private object $object, | |||
|
|
|||
| private bool $shouldPersist = true, | |||
There was a problem hiding this comment.
I think we should directly disable autorefrieshing when shouldPersist is false
There was a problem hiding this comment.
maybe we should also add a condition in methods _delete(), _refresh(). And maybe throw an exception in _disableAutoRefresh()?
I'd make the argument that this is a bug fix rather than a new feature. Having said that, I completely understand the thought process behind merging this into |
| /** | ||
| * @test | ||
| */ |
There was a problem hiding this comment.
because we're using a final class (SomeObject) with the legacy factory class ModelFactory:
| /** | |
| * @test | |
| */ | |
| /** | |
| * @test | |
| * @group legacy | |
| */ |
by the way, shouldn't we add a test using PersistentObjectFactory and ObjectFactory?
|
Hey, sorry for the delay reviewing. I'm inclined to add this to 2.x only. We are really close to a release and it's getting to be a real burden to port stuff from 1.x to 2.x. |
Current behavior:
This is intended to be legacy code that shouldn't be merged / released in 2.x.
fixes: #573