Skip to content
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

Cannot access actual factory instance in after/before Instantiate hooks #677

Open
nikophil opened this issue Jul 24, 2024 · 1 comment · May be fixed by #738
Open

Cannot access actual factory instance in after/before Instantiate hooks #677

nikophil opened this issue Jul 24, 2024 · 1 comment · May be fixed by #738

Comments

@nikophil
Copy link
Member

I recently had a pretty sneaky bug. Given this code:

final class SomeFactory extends PersistentObjectFactory
{
protected function initialize(): static
    {
        return $this->beforeInstantiate(
            function (array $attributes): array {
                if ($this->isPersisting()) {
                    // do some stuff which interacts with db
                } else {
                    // do some stuff without db
                }

                return $attributes;
            }
        );
    }
}

// in some test:
SomeFactory::new()->withoutPersisting()->create();

We're entering in the if() because $this here represents the factory returned by SomeFactory::new() and not the last instance on which we call create(). This is really unlucky, I had hard time to figure what was going on...

My first guess to fix this was to bind the closure to the actual instance used in create():

// SomeFactory
public function create(callable|array $attributes = []): object
{
    // ...

    foreach ($this->beforeInstantiate as $hook) {
        try {
	        $hook = \Closure::fromCallabale($hook)->bindTo($this);
		} catch(\Exception) {
            // the closure is static and cannot be bound to anything
        }

        $parameters = $hook($parameters, static::class());

        // ...
    }
}

it works nicely, but it would not work outside from a factory, so this is a no-go.

Another solution would be to the current factory as a new parameter to all our hooks:

    /**
     * @param callable(Parameters,class-string<T>, static):Parameters $callback
     */
    final public function beforeInstantiate(callable $callback): static;

    /**
     * @final
     *
     * @param callable(T,Parameters, static):void $callback
     */
    public function afterInstantiate(callable $callback): static;

    /**
     * @param callable(T, Parameters, static):void $callback
     */
    final public function afterPersist(callable $callback): static;

WDYT?

@kbond
Copy link
Member

kbond commented Jul 24, 2024

My first guess to fix this was to bind the closure to the actual instance used in

I feel like doing this is always a bit sneaky - it's not obvious this has been done and can cause it's own wtf.

Another solution would be to the current factory as a new parameter to all our hooks:

This feels like the best way to handle.

@nikophil nikophil linked a pull request Dec 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants