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

[11.x] Change scope for afterCreating and afterMaking callbacks #51772

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

jacob418
Copy link
Contributor

Configuration variables/properties on factories can not be used in the afterMaking and afterCreating callbacks because they bind to the wrong scope/instance.

Configuration methods like has, for, state and many more create new instances via the newInstance method. This means, that the afterMaking and afterCreating are called with the scope of a prior instance. In most cases this is no problem unless one wants to use the configuration parameters of the current instance. Any changes, to the factory configuration made after the configure call are trapped in later instances and thus not available in those callbacks.

class MyModelFactory extends \Illuminate\Database\Eloquent\Factories\Factory
{
    public function __construct(
        $count = null,
        ?Collection $states = null,
        ?Collection $has = null,
        ?Collection $for = null,
        ?Collection $afterMaking = null,
        ?Collection $afterCreating = null,
        $connection = null,
        ?Collection $recycle = null,
        protected bool $withMyConfigFlag = false
    ) {
        parent::__construct(
            $count,
            $states,
            $has,
            $for,
            $afterMaking,
            $afterCreating,
            $connection,
            $recycle
        );
    }

    protected function newInstance(array $arguments = [])
    {
        $arguments = array_merge(
            $arguments,
            [
                'withMyConfigFlag' => array_key_exists('withMyConfigFlag', $arguments)
                    ? $arguments['withMyConfigFlag']
                    : $this->withMyConfigFlag,
            ],
        );

        return parent::newInstance($arguments);
    }

    public function withMyConfigFlag(bool $withMyConfigFlag = true): static
    {
        $this->withMyConfigFlag = $withMyConfigFlag;

        return $this->newInstance(['withMyConfigFlag' => $withMyConfigFlag]);
    }

    public function configure(): static
    {
        return $this->afterMaking(function (MyModel $myModel) {
            if ($this->withMyConfigFlag) { // will always be `false`
              // do something...
            }
        });
    }
}

This pull request shifts the scope to the current factory instance when calling the callbacks, thus making the configuration of the factory instance available where the make or create method was called.

@crynobone crynobone changed the title Change scope for afterCreating and afterMaking callbacks [11.x] Change scope for afterCreating and afterMaking callbacks Jun 11, 2024
@taylorotwell taylorotwell merged commit 202422d into laravel:11.x Jun 11, 2024
28 checks passed
@mabdullahsari
Copy link
Contributor

This is a breaking change for apps that define static closures. $this cannot be bound in such cases.

  FAILED  Tests\Integration\redacted\Checkout\CheckoutTest > it throws if no basket items were provided                                                                           
  Failed asserting that exception of type "ErrorException" matches expected exception "redacted\Checkout\Exceptions\ItemsMissingException". Message was: "Cannot bind an instance to a static closure"

@donnysim
Copy link
Contributor

donnysim commented Jun 20, 2024

Yeah, this is a breaking change. I usually try to avoid using static closures directly in factories because it's not the only thing that doesn't work with it, but some still get through in seeders etc. which after this it doesn't work. While this is nothing major if only used for tests/seeds, there's no guarantee that someone's not using them more seriously.

@mabdullahsari
Copy link
Contributor

there's no guarantee that someone's not using them more seriously.

We use them in app code, and our test suite caught the breakages. I can't guarantee everyone else is going to be as lucky, though. ¯_(ツ)_/¯

@driesvints
Copy link
Member

We've reverted this PR and released v11.11.1, thanks for reporting.

@jacob418 I don't think this is something we'll be able to do unless we break other people's apps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants