-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature: Domain autoloader #49
Feature: Domain autoloader #49
Conversation
…tions/dependabot/fetch-metadata-2.0.0 Bump dependabot/fetch-metadata from 1.6.0 to 2.0.0
Thanks! I'll take a look at this later today when I have some dedicated time to dive in. I can help out with tests. |
That would be great. I'm also not so familiar with Pest. |
I noticed some problems with this implementation. I will fix them now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand you might still be working on this, but wanted to offer my thoughts so far. I see there is some cache functionality in there. What is the role of caching in all of this and why is it so important that it's worth "rolling our own"?
I haven't pulled anything down, just browsing the source code at this point. I will need a few more passes to grasp everything.
config/ddd.php
Outdated
@@ -60,8 +60,47 @@ | |||
'resource' => 'Resources', | |||
'rule' => 'Rules', | |||
'scope' => 'Scopes', | |||
'factories' => 'Database\Factories', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should normalize to singular form 'factory' like the rest of the namespace keys.
Secondly, am I understanding correctly that this introduces a breaking change in how domain factories are resolved? Previously stored outside the domain layer in database/factories/<domain>/ModelFactory.php
from the base path, now stored inside the respective domain folders?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. That should be singular.
The way it loads now is first from the domain at the specified namespace, and then falls back to database/factories/<domain>/<model>Factory.php
as you can see here in the code:
https://github.com/lunarstorm/laravel-ddd/pull/49/files#diff-f90c0b4b5b547c6f3de3497309fb7e882872d1e90bfa612c5bd98fda13018665R131-R135
I'm open to change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, got it. I personally like the co-location of factories within the domain layer alongside the models. I wonder if this should become the standard behaviour moving forward.
The database/factories/<domain>/<model>Factory.php
convention was based on the book I read a couple years ago, Martin Joo's "Domain-Driven Design With Laravel".
From my experience I would have liked to use the built in cache system in Laravel, but the problem is that the cache drivers are not available during registration of service providers and that is where the service providers needs to be registered. This is also how Laravel is caching service providers, events and routes internally. Some examples from I could do some benchmarks if you want. |
Good to know, I haven't really gone down that rabbit hole of optimization, but makes sense here since we're dealing with autoloading. I was hoping there'd be some way to control the order of execution and have the package service provider loaded after the Laravel services become available - but the register method doesn't work like that, does it? |
config/ddd.php
Outdated
@@ -60,8 +60,47 @@ | |||
'resource' => 'Resources', | |||
'rule' => 'Rules', | |||
'scope' => 'Scopes', | |||
'factory' => 'Database\Factories', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on simplifying this namespace to Factories
instead of Database\Factories
? Or is this following a certain pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is where I have been placing my factories, and from what I have seen in other projects that seems pretty common, although from a small sample size. The Database
folder usually also contain folders for migrations and seeders for that domain, just like the /database
folder in a vanilla Laravel app.
I think that is a sensible default, but because this is easy configure how you want it I have no strong feelings. I would be fine with Factories
as well, and that looks nicer/cleaner in the config.
Are you in the middle of changes or may I pull this down and start tweaking/adding to it? |
No, go ahead and do what you want 👍 |
config/ddd.php
Outdated
| For example: Domain/Invoicing/Database/Factories/InvoiceFactory.php | ||
*/ | ||
'factories' => 'Database\\Factories\\{model}Factory', | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering, are these autoload config strings necessary, now that ddd.namespaces.provider
, ddd.namespaces.command
, ddd.namespaces.policy
, and ddd.namespaces.factory
exist? All these patterns can be derived based on whatever is configured in ddd.namespaces.*
.
Instead, each ddd.autoload.*
can be simplified to booleans only, i.e., whether you want to opt into the autoload behaviour or not. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I actually made them as booleans first, but I thought it would be good to be able to customize. It also still works with booleans,. With true
it will load from the default location. But I agree, true
should probably be the default in the config file.
I was also thinking about supporting to pass an array there if you want to load from multiple locations inside each domain. For example:
'service_providers' => [
'*/*ServiceProvider.php',
'*/Providers/*.php',
],
That might be a bit over-engineered though. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into how lorisleiva/laravel-actions
implements discovery (in their case console commands), and saw that they use https://github.com/lorisleiva/lody. Extremely powerful and greatly simplifies things. It then allows the config to specify folders only (not patterns), and it can take care of the rest.
e.g.,
$finder = Finder::create()->files()->in($paths);
return Lody::classesFromFinder($finder)
->isNotAbstract()
->isInstanceOf(ServiceProvider::class)
->toArray();
I will push my updates soon and summarize everything in more detail there.
Summary of key changes:
Bonus: with Factory auto-discovery in place, generated domain models don't need to extend any base model and can extend |
- refactor autoload internals for better testability - make autoload caching opt-in via ddd:cache, otherwise the initial cache will never be busted - clean up tests
Did a bunch more tests (as well as on a real application) to land on a complete implementation. To sum it up, here's the snippet from the README: Domain Autoloading and DiscoveryAutoloading behaviour can be configured with the 'autoload' => [
'providers' => true,
'commands' => true,
'policies' => true,
'factories' => true,
], Service ProvidersWhen Console CommandsWhen PoliciesWhen FactoriesWhen If your application implements its own factory discovery using Disabling AutoloadingYou may disable autoloading by setting the respective autoload options to // 'autoload' => [
// 'providers' => true,
// 'commands' => true,
// 'policies' => true,
// 'factories' => true,
// ], Autoloading in ProductionIn production, you should cache the autoload manifests using the |
Great work @jaspertey! I will try to migrate my project to this next week. |
* Drop Laravel 9 support * Remove Laravel 9 related shims. * Proxy-generators, WIP. * make:enum only in laravel 11 * Use ddd:* command prefixes. * Update command prefixes. * Refactoring internals, WIP. * Fix styling * Almost all tests passing now. * Formatting. * Fix test. * Change details. * Include next branch. * Switch to standard console prompt. * Bump phpstan php-version to 8.2 * Skip enum tests for < laravel 11 * Update readme. * Standardize generator class names following Laravel's conventions. * Formatting. * ddd:list * Normalize path. * Formatting * Update wordings. * Bump dependabot/fetch-metadata from 1.6.0 to 2.0.0 Bumps [dependabot/fetch-metadata](https://github.com/dependabot/fetch-metadata) from 1.6.0 to 2.0.0. - [Release notes](https://github.com/dependabot/fetch-metadata/releases) - [Commits](dependabot/fetch-metadata@v1.6.0...v2.0.0) --- updated-dependencies: - dependency-name: dependabot/fetch-metadata dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> * Prompts (#48) * Use Laravel Prompts for domain input prompt. * Add version matrix to readme. * Bump dependencies. * Address base-view-model generation issues. * Command aliases. * Feature: Domain autoloader (#49) * Domain autoloading and discovery (providers, commands, policies, factories) --------- Co-authored-by: Jasper Tey <[email protected]> * Update change details. * ddd:upgrade command to help convert outdated 0.x config files. * Generate factories within domain layer. (#52) * Add upgrade notes. --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: JasperTey <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Peter Elmered <[email protected]>
First pass on an domain autoloader implementation.
I would like to add tests for this, but it is tricky to test in isolation without an actual project structure.
TODO
Domain Autoloading and Discovery
Autoloading behaviour can be configured with the
ddd.autoload
configuration option. By default, domain providers, commands, policies, and factories are auto-discovered and registered.Service Providers
When
ddd.autoload.providers
is enabled, any class within the domain layer extendingIlluminate\Support\ServiceProvider
will be auto-registered as a service provider.Console Commands
When
ddd.autoload.commands
is enabled, any class within the domain layer extendingIlluminate\Console\Command
will be auto-registered as a command when running in console.Policies
When
ddd.autoload.policies
is enabled, the package will register a custom policy discovery callback to resolve policy names for domain models, and fallback to Laravel's default for all other cases. If your application implements its own policy discovery usingGate::guessPolicyNamesUsing()
, you should setddd.autoload.policies
tofalse
to ensure it is not overridden.Factories
When
ddd.autoload.factories
is enabled, the package will register a custom factory discovery callback to resolve factory names for domain models, and fallback to Laravel's default for all other cases. Note that this does not affect domain models using theLunarstorm\LaravelDDD\Factories\HasDomainFactory
trait. Where this is useful is with regular models in the domain layer that use the standardIlluminate\Database\Eloquent\Factories\HasFactory
trait.If your application implements its own factory discovery using
Factory::guessFactoryNamesUsing()
, you should setddd.autoload.factories
tofalse
to ensure it is not overridden.Disabling Autoloading
You may disable autoloading by setting the respective autoload options to
false
in the configuration file as needed, or by commenting out the autoload configuration entirely.Autoloading in Production
In production, you should cache the autoload manifests using the
ddd:cache
command as part of your application's deployment process. This will speed up the auto-discovery and registration of domain providers and commands. Theddd:clear
command may be used to clear the cache if needed.