Skip to content

Conversation

@MatanYadaev
Copy link
Contributor

@MatanYadaev MatanYadaev commented Nov 28, 2021

Currently, Collection methods that have a default parameter return incorrect type when the default parameter value is a closure.

The methods are:

  • Collection::first
  • Collection::last
  • Collection::get
  • Collection::pull
  • LazyCollection::first
  • LazyCollection::last
  • LazyCollection::get

Code example:

$users = User::get();

// `$user1` is `User|null` - works well
$user1 = $user->get(key: 0);

// `$user2` is `User` - works well
$user2 = $user->get(key: 0, default: new User());

// `$user3` is `User|(Closure(): User)` - the type isn't correct
$user3 = $user->get(key: 0, default: function () {
  return new User();
});

After this PR:

// `$user4` is `User`
$user4 = $user->get(key: 0, default: function () {
  return new User();
});

Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this pull request. Feedback that must be addressed before merging:

  • Please replace callable by \Closure.
  • Please adjust the types on the interface `Illuminate\Support\Enumerable'.

@MatanYadaev
Copy link
Contributor Author

@nunomaduro Thanks for the review. Done.

@driesvints driesvints marked this pull request as draft November 29, 2021 13:21
@driesvints
Copy link
Member

Hi there, we've temporarily disabled our PostgreSQL build until we've managed to fix it. I've marked your PR as draft for now. Please rebase with master and mark this PR as ready when all tests pass again. Thanks.

@MatanYadaev MatanYadaev marked this pull request as ready for review November 29, 2021 14:40
@taylorotwell taylorotwell merged commit ee5efbc into laravel:master Nov 29, 2021
@driesvints
Copy link
Member

@MatanYadaev you incorrectly rebased this PR, causing some commits to duplicate. Luckily this PR was squash-merged. Please be mindful when rebasing PR's to avoid this as this can cause real-havoc on projects.

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.

4 participants