-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Attributes added in beforeInstantiate
are not passed to afterPersist
#634
base: 2.x
Are you sure you want to change the base?
Conversation
74abce2
to
86012dd
Compare
* @internal | ||
* @var Parameters|null | ||
*/ | ||
protected array|null $normalizedParameters = null; |
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.
@nikophil, don't we need to set this back to null
on clone?
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.
need to test this
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.
ok, so... an easy test like this one (that I added to our test suite) was failing because of the memoization:
public function can_create_different_objects_based_on_same_factory(): void
{
$factory = Object1Factory::new(['prop1' => 'first object']);
$object1 = $factory->create();
self::assertSame('first object-constructor', $object1->getProp1());
$object2 = $factory->create(['prop1' => 'second object']);
self::assertSame('second object-constructor', $object2->getProp1());
$object3 = $factory->with(['prop1' => 'third object'])->create();
self::assertSame('third object-constructor', $object3->getProp1());
}
I'm really not proud of the solution I found (replace $this->normalizedParameters ??=
by $this->normalizedParameters =
in Factory::normalizeParameters()
, but it's the only simple one I found 🤷
86012dd
to
093330a
Compare
093330a
to
5e44ec8
Compare
Followup to quick fix in #631 that demonstrates an edge case it didn't fix.