Skip to content

[10.x] Update collection docs to use template-covariant for the values #46866

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

Conversation

Rocksheep
Copy link
Contributor

This PR will allow developers to use covariants in their collections. Before this was not possible and you would get errors from PHPStan. This is also an issue in older versions of Laravel. The following example would throw PHPStan errors

class Animal {}
class Tiger extends Animal {}
class Lion extends Animal {}
class Zebra extends Animal {}

class Zoo {
    /**
     * @var \Illuminate\Support\Collection<int, Animal>
     */
    private Collection $animals;

    public function __construct() {
        $this->animals = collect([
            new Tiger,
            new Lion,
            new Zebra,
        ]);
    }

    /**
     * @return \Illuminate\Support\Collection<int, Animal>
     */
    public function getWithoutZebras(): Collection {
        return $this->animals->filter(fn (Animal $animal) => ! $animal instanceof Zebra);
    }
}

The thrown error would be

Property Zoo::$animals (Illuminate\Support\Collection<int, Animal>) does not accept Illuminate\Support\Collection<int, Lion|Tiger|Zebra>.

Changing the @template to @template-covariant fixes this.

This allows developers to use classes with inheritance on collections. See types/Support/Collection.php for an example
@Rocksheep Rocksheep force-pushed the feature/add-covariant-to-generic-template-dock branch from 08719f4 to bc19a07 Compare April 24, 2023 13:57
@nunomaduro nunomaduro marked this pull request as draft April 24, 2023 14:44
@nunomaduro
Copy link
Member

@Rocksheep Does PHPStorm and Psalm understand that @template-covariant?

@Rocksheep
Copy link
Contributor Author

@nunomaduro Yeah! Psalm definitely has support for it as can be read on their documentation and here are screenshots to display that PHPStorm also understands it.
afbeelding
afbeelding

@Rocksheep Rocksheep marked this pull request as ready for review April 24, 2023 14:58
@nunomaduro nunomaduro marked this pull request as draft April 24, 2023 15:07
@nunomaduro
Copy link
Member

Please don't set the pull request "ready for review" while we are discussing it.

  1. Why this change is not being applied to Illuminate/Collections/helpers.php, Illuminate/Database/Eloquent/Collection.php, Illuminate/Database/Eloquent/Casts/ArrayObject.php? Is it intentional?
  2. Also, why we need this change on the Illuminate/Contracts/Support/Arrayable.php?

@Rocksheep
Copy link
Contributor Author

Ah sorry I thought I had pushed it out of the review state because of a push I did earlier. I didn't add it to the database collections because I didn't run into issues with that yet, because I don't usually pull multiple models from the database in a single call.

So I have only changed the files where I ran into the problem and I missed the helper.

And about that arrayable. I thought it would have the same problem, but checking now it doesn't seem so. I will remove it and add it to the collection helpers.

Do you think the other classes you mentioned need the template change?

@nunomaduro nunomaduro changed the base branch from 10.x to feat/covariant-collections April 24, 2023 18:33
@nunomaduro nunomaduro marked this pull request as ready for review April 24, 2023 18:33
@nunomaduro nunomaduro merged commit 6ce67bd into laravel:feat/covariant-collections Apr 24, 2023
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