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

fix: Added CreatesApplication strict typing #1626

Merged

Conversation

JanMikes
Copy link
Contributor

@phanan
Copy link
Member

phanan commented Dec 15, 2022

Thanks, but E2E isn't failing because of this (which actually I'm not sure what it is TBH). It's failing because I don't have time yet to adapt it to the vue 3 rewrite.

@phanan phanan closed this Dec 15, 2022
@JanMikes
Copy link
Contributor Author

JanMikes commented Dec 15, 2022

Hi, thank you for your reply.

Maybe we misunderstood each other - E2E tests of larastan are failing and it has nothing to do with vue 3 rewrite: https://github.com/nunomaduro/larastan/actions/runs/3684486433/jobs/6234278865

My change in this MR fixes this error:

 ------ -------------------------------------------------------------------------------------------------------------
  Line   tests/Traits/CreatesApplication.php (in context of class Tests\TestCase)
 ------ -------------------------------------------------------------------------------------------------------------
  23     Property Tests\TestCase::$artisan (App\Console\Kernel) does not accept Illuminate\Contracts\Console\Kernel.
 ------ -------------------------------------------------------------------------------------------------------------


 [ERROR] Found 1 error

This error is introduced with change in larastan - dynamic extension which makes phpstan understand return type of $this->app->make().

It is clear that the code in \Tests\Traits\CreatesApplication is trying to put interface type into variable with specific implementation type, which makes phpstan fail (correctly!).

private Kernel $artisan;

$this->artisan = $app->make(Artisan::class);

Adding assertion there, makes 100% sure (and phpstan understands) that $app->make(Artisan::class) returns Kernel:

$artisan = $app->make(Artisan::class);
assert($artisan instanceof Kernel);

$this->artisan = $artisan;

I tested everything locally and it works very well.

Can we please reopen and re-consider this MR? Thank you.

@JanMikes
Copy link
Contributor Author

JanMikes commented Dec 15, 2022

Other possible solution (and maybe more readable) would be to do this:

- $this->artisan = $app->make(Artisan::class);
+ $this->artisan = $app->make(Kernel::class);

But honestly i did not know what (and if any) was intention of getting Artisan::class from DI, so i kept it that way.

@phanan phanan reopened this Dec 15, 2022
@phanan
Copy link
Member

phanan commented Dec 15, 2022

In such a case, wouldn't it be simpler to use type-hinting instead?

/** @var Kernel $artisan */
$artisan = $app->make(Artisan::class);

@JanMikes
Copy link
Contributor Author

👍 This is alternative, as well possible way

@phanan
Copy link
Member

phanan commented Dec 15, 2022

…which I'd prefer, because it doesn't cost anything :) Would you mind revising the PR accordingly?

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2022

Codecov Report

Merging #1626 (113633c) into master (e8e2c2d) will not change coverage.
The diff coverage is n/a.

❗ Current head 113633c differs from pull request most recent head 7c666c5. Consider uploading reports for the commit 7c666c5 to get more accurate results

@@            Coverage Diff            @@
##             master    #1626   +/-   ##
=========================================
  Coverage     81.50%   81.50%           
  Complexity      654      654           
=========================================
  Files           151      151           
  Lines          1757     1757           
=========================================
  Hits           1432     1432           
  Misses          325      325           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@JanMikes
Copy link
Contributor Author

Updated. Thank you for being open for this kind of improvements.

@phanan phanan changed the title Added CreatesApplication strict typing fix: Added CreatesApplication strict typing Dec 15, 2022
@phanan phanan merged commit 9e864c9 into koel:master Dec 15, 2022
@phanan
Copy link
Member

phanan commented Dec 15, 2022

All good, thanks a lot!

@JanMikes JanMikes deleted the fix-larastan-createsApplication-kernel-type branch December 15, 2022 16:36
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.

None yet

3 participants