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

[10.x] Update Kernel::load() to use same classFromFile logic as events #47327

Merged
merged 2 commits into from Jun 2, 2023

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented Jun 2, 2023

Closes #47312

Replaces the logic the Console\Kernel uses to parse classes from files with the same logic used by Events in DiscoverEvents::classFromFile()---this allows commands outside the App namespace to be auto-registered if desired.

Thanks!

@driesvints
Copy link
Member

I don't feel we should do this. I don't think we should encourage people to move these files outside the default app directory.

@calebdw
Copy link
Contributor Author

calebdw commented Jun 2, 2023

Why not? I work on a large application and we follow a DDD approach inside another namespace, there's no reason for an artificial restriction that all commands must be inside the app directory.

I just updated the logic to be consistent with the logic already being used when auto-registering Listeners

@calebdw
Copy link
Contributor Author

calebdw commented Jun 2, 2023

Note that this was just a quick, in-place edit, but longer term it might be a good idea to move this logic to a DiscoverCommands class similar to DiscoverEvents (possibly with the common logic abstracted to a DiscoverClasses class that both of them extend)

@taylorotwell taylorotwell merged commit 2b174d5 into laravel:10.x Jun 2, 2023
16 checks passed
@calebdw
Copy link
Contributor Author

calebdw commented Jun 2, 2023

Thanks @taylorotwell!

@taylorotwell
Copy link
Member

Reverted: #47377

@devlob
Copy link

devlob commented Jul 24, 2023

I agree, I'm also using Laravel as my Infrastructure on a project that I use DDD and I was shocked that the commands for autoloading would be hardcoded to app, I want the freedom to change that as a developer.

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.

Cannot auto-register commands in non-default namespace with multiple namespaces
4 participants