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: make, makeWith and resolve methods not resolving to correct type on Application and Container. #1451

Merged
merged 3 commits into from Dec 29, 2022

Conversation

mad-briller
Copy link
Contributor

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

resolves #1441

Changes
Added return type extension for Application and Container make, makeWith and resolves method.

Abstracts repeated behavior into a AppMakeHelper and uses that in places i could find it being duplicated.

if ($expr instanceof String_) {
try {
/** @var object|null $resolved */
$resolved = $this->resolve($expr->value);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not really guaranteed that resolve will return an object, scalars can be bound into the container using constant string names too and i've seen that in the wild, so it might be worth expanding this code to anticipate more types to be returned in the future

@mad-briller
Copy link
Contributor Author

mad-briller commented Nov 21, 2022

not sure that the koel/koel e2e failures are caused by this change

@mad-briller mad-briller force-pushed the bug-1441 branch 2 times, most recently from 274fc1f to fc206a7 Compare November 30, 2022 12:04
@JanMikes
Copy link

JanMikes commented Dec 11, 2022

Hi, i have checked the failing E2E test and actually this is correct that test is failing.

https://github.com/koel/koel/blob/master/tests/Traits/CreatesApplication.php#L25

    use App\Console\Kernel;
    use Illuminate\Contracts\Console\Kernel as Artisan;

    private Kernel $artisan;

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

Because this PR adds phpstan functionality that makes it understand that $app->make(Artisan::class) will return instance of Artisan (the interface) and the property is typed to specific implementation.

I will try send a PR to fix this issue in original repo.

@mad-briller
Copy link
Contributor Author

@canvural is there anything i can do to help get this over the line? thanks

@JanMikes
Copy link

Pls can you re-run the E2E pipeline? Should be passing now.

@szepeviktor
Copy link
Collaborator

Pls can you re-run the E2E pipeline?

Here you go. This is the rerun.
https://github.com/nunomaduro/larastan/actions/runs/3684486433/jobs/6280772909

@JanMikes
Copy link

I see - though the dependent PR is merged, there is fixed git reference: https://github.com/nunomaduro/larastan/blob/master/.github/workflows/e2e-tests.yml#L40

We would need to change it to 9e864c937f5f060aa0192596d4e0776e68dc16e5 (references to koel/koel@9e864c9)

@canvural canvural changed the title Fix make(), makeWith() and resolve() methods not resolving to correct type on Application and Container. fix: make, makeWith and resolve methods not resolving to correct type on Application and Container. Dec 29, 2022
@canvural canvural merged commit d304b77 into larastan:master Dec 29, 2022
@canvural
Copy link
Collaborator

Thank you!

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.

Illuminate\Foundation\Application in Service provider closure type not resolving
4 participants