Skip to content

feat: add type hints for $this in routes/console.php #6554

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

Closed
wants to merge 1 commit into from

Conversation

AJenbo
Copy link
Contributor

@AJenbo AJenbo commented Feb 26, 2025

Summary

This PR adds a type hint for $this in routes/console.php, ensuring better IDE support and improved static analysis when working with console commands.

Why This Change?

  • Improves Developer Experience: IDEs can now provide autocompletion and proper type inference for $this within console command closures.
  • Enhances Static Analysis: Helps tools like PHPStan understand the type of $this, reducing potential false positives in type checks.
  • Better Code Readability: Explicitly declaring $this as an instance of Illuminate\Console\Command makes the code easier to understand for new developers.

Testing & Compatibility

  • No runtime behavior changes; this is a documentation-level improvement.
  • Fully backward compatible with existing Laravel applications.

@AJenbo AJenbo force-pushed the this-console-route branch from d8f2dc3 to eec85ab Compare February 26, 2025 22:59
use Illuminate\Foundation\Inspiring;
use Illuminate\Support\Facades\Artisan;

/** @var Command $this */

Choose a reason for hiding this comment

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

Using the FQN \Illuminate\Console\Command here and removing the import would be better IMO, the class isn't actually used in any code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@AJenbo AJenbo force-pushed the this-console-route branch from a947dd1 to 63360d7 Compare February 28, 2025 11:48
@AJenbo
Copy link
Contributor Author

AJenbo commented Feb 28, 2025

@bert-w this solves the issue you where having with route files missing context for $this.

@timacdonald
Copy link
Member

timacdonald commented Mar 2, 2025

The proposed change sets $this within the entire routes/console.php file to be an instance of Illuminate\Console\Command.

However, $this is an instance of Illuminate\Foundation\Console\Kernel in the routes/console.php file.

<?php

use Illuminate\Foundation\Inspiring;
use Illuminate\Support\Facades\Artisan;

Artisan::command('inspire', function () {
    $this->comment(Inspiring::quote());
})->purpose('Display an inspiring quote');

dd($this::class);
Screenshot 2025-03-02 at 16 12 48

If this was wanted, the docblock would need to be contained within each command's anonymous Closure, otherwise it is invalid and would remove the tooling's ability to help.

- /** @var Illuminate\Console\Command $this */
Artisan::command('inspire', function () {
+   /** @var Illuminate\Console\Command $this */
    $this->comment(Inspiring::quote());
})->purpose('Display an inspiring quote');

@AJenbo
Copy link
Contributor Author

AJenbo commented Mar 2, 2025

Hmm I se what's happening is that the context is rebound between the function being defined and later being executed:

https://github.com/laravel/framework/blob/7b8e12d8f4218615370d329a258f0b7c4afc6237/src/Illuminate/Foundation/Console/ClosureCommand.php#L64

This change is going to be obscure to a lot of tools which expects the context to be bound at the time the closure is being defined and so expect that it's using the context of the file. I don't think your solution of adding the docblock inside of the anonymous function is going to work in general for the same reason.

I'll close this PR for now.

@AJenbo AJenbo closed this Mar 2, 2025
@AJenbo AJenbo deleted the this-console-route branch March 2, 2025 18:43
@AJenbo AJenbo restored the this-console-route branch March 2, 2025 18:56
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.

3 participants