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

Modifying collection's shape with combine() seems not supported #1053

Closed
underdpt opened this issue Nov 26, 2021 · 4 comments
Closed

Modifying collection's shape with combine() seems not supported #1053

underdpt opened this issue Nov 26, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@underdpt
Copy link

underdpt commented Nov 26, 2021

  • Larastan Version: 1.0.2
  • --level used: 8
  • Pull request with failing test: none

Description

Running the code below, I get:

Parameter #1 $callback of method Illuminate\Support\Collection<int,string>::mapWithKeys() 
expects callable(string, int): array, 
Closure(stdClass, string): non-empty-array<string, stdClass> given.

I think this is because the combine() method changes the shape of the collection. I didn't find that method on the stubs, so maybe this is too much magic for larastan.

I tried adding a phpdoc after the combine() method to inform larastan about the new shape, but it's not working, is there any way to inform phpstan about the new shape of the collection?

public function test(): void
{
    $row = [new \stdClass(), new \stdClass()];

    $new = collect(['key1', 'key2'])
        ->combine($row)
        /** @var \Illuminate\Support\Collection<\stdClass, int> $row */
        ->mapWithKeys(fn (\stdClass $value, string $key) => [$key => $value]);
}

Laravel code where the issue was found

public function test(): void
{
    $row = [new \stdClass(), new \stdClass()];

    $new = collect(['key1', 'key2'])
        ->combine($row)
        ->mapWithKeys(fn (\stdClass $value, string $key) => [$key => $value]);
}
@szepeviktor
Copy link
Collaborator

szepeviktor commented Nov 26, 2021

Hallo David! 👋
Larastan infers closure parameters from the collection. combine is not taken into consideration.
https://github.com/nunomaduro/larastan/blob/695d005f56d2fd71876d604e78708ad1acc96781/stubs/Enumerable.stub#L28

    $combined = collect(['key1', 'key2'])->combine($row);
    /** @var \Illuminate\Support\Collection<string, \stdClass> $combined */
    $new = $combined->mapWithKeys(fn (\stdClass $value, string $key) => [$key => $value]);

How about something like this? I may have made a mistake!

@underdpt
Copy link
Author

underdpt commented Dec 2, 2021

Hello and thanks for your answer!

Will use that as a workaround, better than ignoring the line. It would be good that Larastan could understand combine so we could maintain the fluent syntax. Do you think this can be done? I have not enough knowledge but if this is doable, I can put this into my TODO list (no promises, sorry!)

@szepeviktor szepeviktor added the enhancement New feature or request label Dec 2, 2021
@szepeviktor
Copy link
Collaborator

Do you think this can be done?

Definitely. Larastan does it with few dozen methods.

@canvural
Copy link
Collaborator

canvural commented Feb 1, 2022

Hi,

As of Larastan 2 all the collection related stubs are moved to Laravel core. So please test this with Laravel 9 and Larastan 2, and if the error still happens please open an issue on Laravel repo. Thanks.

@canvural canvural closed this as completed Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants