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

[10.x] Fix Factory::configure() return type #47920

Merged
merged 1 commit into from
Aug 1, 2023
Merged

[10.x] Fix Factory::configure() return type #47920

merged 1 commit into from
Aug 1, 2023

Conversation

axlon
Copy link
Contributor

@axlon axlon commented Aug 1, 2023

TL;DR: Small doc block fix

The Factory::configure() method currently indicates that it returns $this (the exact instance the method was called on), but this is not always true. For example, when calling the afterCreating() method a new factory instance is created, e.g.:

    /**
     * @return static
     */
    public function configure(): static
    {
        return $this->afterCreating(static function (MyModel $model) {
            doSomethingWithTheModel($model);
        });
    }

For most IDEs this isn't an issue because they will handle $this and static the same way, but tools like PHPStan do differentiate between them. When using PHPStan strict rules, you would get an error here because the @return static in the example doesn't comply with the @return $this on the base factory, and if you changed it to @return $this it will also fail because you are in fact returning a new instance (the return type for afterCreating() is documented as static).

This PR solves this by fixing the return type on the configure method, this is not a breaking change as its only a docblock type and static encompasses $this so anyone using @return $this is still fine.

@taylorotwell taylorotwell merged commit 872b086 into laravel:10.x Aug 1, 2023
@axlon axlon deleted the factory-configure-patch branch August 1, 2023 13:53
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.

2 participants