-
-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -59,6 +59,8 @@ class Factory | |||
/** @var callable[] */ | |||
private array $afterPersist = []; | |||
|
|||
private bool $shouldPersist = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't it possible to directly use $this->persist
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
private static function normalizeObject(object $object): Proxy |
$this->persist
was a no-go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should directly disable autorefrieshing when shouldPersist
is false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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