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

Make commands lazy #12048

Merged
merged 4 commits into from
Mar 28, 2024
Merged

Conversation

stephenjude
Copy link
Contributor

@stephenjude stephenjude commented Mar 26, 2024

Description

This PR makes the Filament commands "lazy", i.e., they do not need to be instantiated when running other commands.

Currently, when you have Filament installed in a Laravel project, if you were to run php artisan filament:install all of the Filament commands are instantiated. This is just how Symfony console commands work.

The AsCommand attribute was introduced to allow commands to be "lazy", i.e., they do not need to be instantiated when running other commands.

@timacdonald has been doing some work across the Laravel ecosystem to standardise this and I thought to drop by here and do the same.

See: laravel/framework#50617

Functional changes

  • Code style has been fixed by running the composer cs command.
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@danharrin danharrin added the enhancement New feature or request label Mar 27, 2024

#[AsCommand(name: 'make:filament-exporter', aliases: ['filament:exporter'])]
Copy link
Member

Choose a reason for hiding this comment

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

For the alias command classes, do the arguments need to be swapped? So make:filament-exporter is an alias of filament:exporter? Maybe I am understanding it wrong, but Symfony's docs do not explain this

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we dont need aliases: at all yet since we aren't using the $aliases property on the commands? These could be treated as separate commands where the alias classes just reference their own name?

Then we refactor to $aliases and get rid of these classes in v4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think we should treat these alias as separate command. I will go ahead and do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the alias command classes, do the arguments need to be swapped? So make:filament-exporter is an alias of filament:exporter? Maybe I am understanding it wrong, but Symfony's docs do not explain this

For swapping of the arguments, I don't think so but I will test it out and see where that leads me.

placeholder: 'Products/ListProducts',
required: true,
))
$component = (string) str(
Copy link
Member

Choose a reason for hiding this comment

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

Please remove all formatting changes in this file and also MakeWidgetCommand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could this formatting be as a result of composer cs command or maybe from my IDE formatting.

@danharrin danharrin added this to the v3 milestone Mar 27, 2024
@stephenjude
Copy link
Contributor Author

stephenjude commented Mar 27, 2024

@danharrin I have made all necessary changes and rebased the commits into one. I think this PR is ready. Let me know if there anything else that needs my attention.

@danharrin danharrin merged commit 687f29f into filamentphp:3.x Mar 28, 2024
10 checks passed
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
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants