From 976889e09858bb2524fe751bf1662fcc19810e1b Mon Sep 17 00:00:00 2001 From: Jasper Tey Date: Sun, 12 Nov 2023 10:08:29 -0500 Subject: [PATCH 1/4] WIP: - skip base model creation if ddd.base_model is a class that exists - otherwise, attempt to generate one, as long as it's within a domain-level namespace --- src/Commands/MakeModel.php | 37 +++++++++++++++++++++++-------- stubs/model.php.stub | 2 +- tests/Generator/MakeModelTest.php | 15 ++++++++++++- 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/src/Commands/MakeModel.php b/src/Commands/MakeModel.php index 51716a4..224ada9 100644 --- a/src/Commands/MakeModel.php +++ b/src/Commands/MakeModel.php @@ -48,15 +48,40 @@ protected function getRelativeDomainNamespace(): string return config('ddd.namespaces.models', 'Models'); } + protected function preparePlaceholders(): array + { + $baseClass = config('ddd.base_model'); + $baseClassName = class_basename($baseClass); + + return [ + 'extends' => filled($baseClass) ? " extends {$baseClassName}" : '', + ]; + } + public function handle() { - $baseModel = config('ddd.base_model'); + $this->createBaseModelIfNeeded(config('ddd.base_model')); + + parent::handle(); + + if ($this->option('factory')) { + $this->createFactory(); + } + } + + protected function createBaseModelIfNeeded($baseModel) + { + if (class_exists($baseModel)) { + return; + } $parts = str($baseModel)->explode('\\'); $baseModelName = $parts->last(); $baseModelPath = $this->getPath($baseModel); - if (! file_exists($baseModelPath)) { + // base_path(config('ddd.paths.domains') . '/Shared/Models/BaseModel.php') + + if (!file_exists($baseModelPath)) { $this->warn("Base model {$baseModel} doesn't exist, generating..."); $this->call(MakeBaseModel::class, [ @@ -64,19 +89,13 @@ public function handle() 'name' => $baseModelName, ]); } - - parent::handle(); - - if ($this->option('factory')) { - $this->createFactory(); - } } protected function createFactory() { $this->call(MakeFactory::class, [ 'domain' => $this->getDomain(), - 'name' => $this->getNameInput().'Factory', + 'name' => $this->getNameInput() . 'Factory', '--model' => $this->qualifyClass($this->getNameInput()), ]); } diff --git a/stubs/model.php.stub b/stubs/model.php.stub index db53f28..f8ff6e8 100644 --- a/stubs/model.php.stub +++ b/stubs/model.php.stub @@ -5,7 +5,7 @@ namespace {{ namespace }}; use {{ rootNamespace }}\Shared\Models\BaseModel; use Illuminate\Database\Eloquent\SoftDeletes; -class {{ class }} extends BaseModel +class {{ class }}{{ extends }} { use SoftDeletes; diff --git a/tests/Generator/MakeModelTest.php b/tests/Generator/MakeModelTest.php index 55628ae..e13b8d5 100644 --- a/tests/Generator/MakeModelTest.php +++ b/tests/Generator/MakeModelTest.php @@ -106,7 +106,7 @@ expect(file_exists($expectedModelPath))->toBeTrue(); })->with('makeModelInputs'); -it('generates the base model if needed', function () { +it('generates the base model when possible', function () { $modelName = Str::studly(fake()->word()); $domain = Str::studly(fake()->word()); @@ -141,6 +141,19 @@ expect(file_exists($expectedBaseModelPath))->toBeTrue(); }); +it('skips base model creation if configured base model already exists', function ($baseModel) { + Config::set('ddd.base_model', $baseModel); + + expect(class_exists($baseModel))->toBeTrue(); + + Artisan::call('ddd:model Fruits Lemon'); + + expect(Artisan::output())->not->toContain("Base model {$baseModel} doesn't exist, generating..."); +})->with([ + ['Illuminate\Database\Eloquent\Model'], + ['Lunarstorm\LaravelDDD\Models\DomainModel'], +]); + it('shows meaningful hints when prompting for missing input', function () { $this->artisan('ddd:model') ->expectsQuestion('What is the domain?', 'Utility') From f94768c6314e117758f25ef62de129ce1fc9ead4 Mon Sep 17 00:00:00 2001 From: JasperTey Date: Sun, 12 Nov 2023 15:08:48 +0000 Subject: [PATCH 2/4] Fix styling --- src/Commands/MakeModel.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Commands/MakeModel.php b/src/Commands/MakeModel.php index 224ada9..8348ab9 100644 --- a/src/Commands/MakeModel.php +++ b/src/Commands/MakeModel.php @@ -81,7 +81,7 @@ protected function createBaseModelIfNeeded($baseModel) // base_path(config('ddd.paths.domains') . '/Shared/Models/BaseModel.php') - if (!file_exists($baseModelPath)) { + if (! file_exists($baseModelPath)) { $this->warn("Base model {$baseModel} doesn't exist, generating..."); $this->call(MakeBaseModel::class, [ @@ -95,7 +95,7 @@ protected function createFactory() { $this->call(MakeFactory::class, [ 'domain' => $this->getDomain(), - 'name' => $this->getNameInput() . 'Factory', + 'name' => $this->getNameInput().'Factory', '--model' => $this->qualifyClass($this->getNameInput()), ]); } From ebade11380a2c227f2aaad2a60fecb3d3196e10e Mon Sep 17 00:00:00 2001 From: Jasper Tey Date: Sun, 12 Nov 2023 11:06:37 -0500 Subject: [PATCH 3/4] Improve the support for base models. --- CHANGELOG.md | 11 ++++++++ src/Commands/MakeModel.php | 28 +++++++++++++++----- src/Support/DomainResolver.php | 17 +++++++++++- tests/Generator/MakeModelTest.php | 43 +++++++++++++++++++++---------- 4 files changed, 79 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 316a5ba..bb4b827 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,17 @@ All notable changes to `laravel-ddd` will be documented in this file. +## [Unversioned] +### Changed +- Implement more robust handling of base models when generating a domain model with `ddd:model`: + - If the configured base model class in `ddd.base_model` exists (evaluated using `class_exists`), base model creation is skipped. + - If the configured base model class does not exist, base model will be created, but only if the class falls under a domain scope. + - Examples of falling under domain scope: `Domain\Shared\Models\CustomDomainBaseModel`, `Domain\Infrastructure\Models\DomainModel`. + - Examples of not falling under domain scope: `App\Models\CustomAppBaseModel`, `Illuminate\Database\Eloquent\NonExistentModel`. + +### Fixed +- Resolve long-standing issue where `ddd:model` would not properly detect whether the configured basemodel in `ddd.base_model` already exists, leading to unpredictable results when `ddd.base_model` deviated from the default `Domain\Shared\Models\BaseModel`. + ## [0.7.0] - 2023-10-22 ### Added - Formal support for subdomains (nested domains). For example, to generate model `Domain\Reporting\Internal\Models\InvoiceReport`, the domain argument can be specified with dot notation: `ddd:model Reporting.Internal InvoiceReport`. Specifying `Reporting/Internal` or `Reporting\\Internal` will also be accepted and normalized to dot notation internally. diff --git a/src/Commands/MakeModel.php b/src/Commands/MakeModel.php index 8348ab9..72d649e 100644 --- a/src/Commands/MakeModel.php +++ b/src/Commands/MakeModel.php @@ -2,6 +2,7 @@ namespace Lunarstorm\LaravelDDD\Commands; +use Lunarstorm\LaravelDDD\Support\DomainResolver; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputOption; @@ -75,17 +76,32 @@ protected function createBaseModelIfNeeded($baseModel) return; } - $parts = str($baseModel)->explode('\\'); - $baseModelName = $parts->last(); - $baseModelPath = $this->getPath($baseModel); + $this->warn("Configured base model {$baseModel} doesn't exist."); + + // If the base model is out of scope, we won't attempt to create it + // because we don't want to interfere with outside folders. + $allowedRootNamespaces = [ + $this->rootNamespace(), + ]; + + if (! str($baseModel)->startsWith($allowedRootNamespaces)) { + return; + } - // base_path(config('ddd.paths.domains') . '/Shared/Models/BaseModel.php') + $domain = DomainResolver::fromClass($baseModel); + + if (! $domain) { + return; + } + + $baseModelName = class_basename($baseModel); + $baseModelPath = $this->getPath($baseModel); if (! file_exists($baseModelPath)) { - $this->warn("Base model {$baseModel} doesn't exist, generating..."); + $this->info("Generating {$baseModel}..."); $this->call(MakeBaseModel::class, [ - 'domain' => 'Shared', + 'domain' => $domain->domain, 'name' => $baseModelName, ]); } diff --git a/src/Support/DomainResolver.php b/src/Support/DomainResolver.php index 43e075d..432d6ec 100644 --- a/src/Support/DomainResolver.php +++ b/src/Support/DomainResolver.php @@ -2,9 +2,24 @@ namespace Lunarstorm\LaravelDDD\Support; +use Illuminate\Support\Str; + class DomainResolver { - public static function fromModelClass(string $modelClass) + public static function fromClass(string $class): ?Domain { + $domainNamespace = basename(config('ddd.paths.domains')).'\\'; + + if (! Str::startsWith($class, $domainNamespace)) { + // Not a domain model + return null; + } + + $domain = str($class) + ->after($domainNamespace) + ->before('\\') + ->toString(); + + return new Domain($domain); } } diff --git a/tests/Generator/MakeModelTest.php b/tests/Generator/MakeModelTest.php index e13b8d5..0028eec 100644 --- a/tests/Generator/MakeModelTest.php +++ b/tests/Generator/MakeModelTest.php @@ -106,10 +106,12 @@ expect(file_exists($expectedModelPath))->toBeTrue(); })->with('makeModelInputs'); -it('generates the base model when possible', function () { +it('generates the base model when possible', function ($baseModelClass, $baseModelPath) { $modelName = Str::studly(fake()->word()); $domain = Str::studly(fake()->word()); + Config::set('ddd.base_model', $baseModelClass); + $expectedModelPath = base_path(implode('/', [ config('ddd.paths.domains'), $domain, @@ -123,23 +125,36 @@ expect(file_exists($expectedModelPath))->toBeFalse(); - // This currently only tests for the default base model - $expectedBaseModelPath = base_path(config('ddd.paths.domains').'/Shared/Models/BaseModel.php'); + $expectedBaseModelPath = base_path($baseModelPath); if (file_exists($expectedBaseModelPath)) { unlink($expectedBaseModelPath); } - // Todo: should bypass base model creation if - // a custom base model is being used. - // $baseModel = config('ddd.base_model'); - - expect(file_exists($expectedBaseModelPath))->toBeFalse(); + expect(file_exists($expectedBaseModelPath))->toBeFalse("{$baseModelPath} expected not to exist."); Artisan::call("ddd:model {$domain} {$modelName}"); - expect(file_exists($expectedBaseModelPath))->toBeTrue(); -}); + expect(file_exists($expectedBaseModelPath))->toBeTrue("{$baseModelPath} expected to exist."); +})->with([ + ['Domain\\Shared\\Models\\BaseModel', 'src/Domain/Shared/Models/BaseModel.php'], + ['Domain\\Infrastructure\\Models\\BaseModel', 'src/Domain/Infrastructure/Models/BaseModel.php'], +]); + +it('will not create a base model if the configured base model is out of scope', function ($baseModel) { + Config::set('ddd.base_model', $baseModel); + + expect(class_exists($baseModel))->toBeFalse(); + + Artisan::call('ddd:model Fruits Lemon'); + + expect(Artisan::output()) + ->toContain("Configured base model {$baseModel} doesn't exist.") + ->not->toContain("Generating {$baseModel}"); +})->with([ + ['Illuminate\\Database\\Eloquent\\NonExistentModel'], + ['OtherVendor\\OtherPackage\\Models\\OtherModel'], +]); it('skips base model creation if configured base model already exists', function ($baseModel) { Config::set('ddd.base_model', $baseModel); @@ -148,10 +163,12 @@ Artisan::call('ddd:model Fruits Lemon'); - expect(Artisan::output())->not->toContain("Base model {$baseModel} doesn't exist, generating..."); + expect(Artisan::output()) + ->not->toContain("Configured base model {$baseModel} doesn't exist.") + ->not->toContain("Generating {$baseModel}"); })->with([ - ['Illuminate\Database\Eloquent\Model'], - ['Lunarstorm\LaravelDDD\Models\DomainModel'], + ['Illuminate\\Database\\Eloquent\\Model'], + ['Lunarstorm\\LaravelDDD\\Models\\DomainModel'], ]); it('shows meaningful hints when prompting for missing input', function () { From 77538cb2f74d108325948f1d4c5e1b35bbc0560b Mon Sep 17 00:00:00 2001 From: Jasper Tey Date: Sun, 12 Nov 2023 22:06:42 -0500 Subject: [PATCH 4/4] Fix tests. --- CHANGELOG.md | 13 ++++++++----- composer.json | 1 + src/Commands/MakeModel.php | 17 ++++++++++------- src/Support/DomainResolver.php | 8 +++----- stubs/model.php.stub | 2 +- tests/Generator/MakeModelTest.php | 31 +++++++++++++++++++++++-------- tests/Model/FactoryTest.php | 2 ++ tests/TestCase.php | 5 ++++- 8 files changed, 52 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb4b827..658db4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,13 +5,16 @@ All notable changes to `laravel-ddd` will be documented in this file. ## [Unversioned] ### Changed - Implement more robust handling of base models when generating a domain model with `ddd:model`: - - If the configured base model class in `ddd.base_model` exists (evaluated using `class_exists`), base model creation is skipped. - - If the configured base model class does not exist, base model will be created, but only if the class falls under a domain scope. - - Examples of falling under domain scope: `Domain\Shared\Models\CustomDomainBaseModel`, `Domain\Infrastructure\Models\DomainModel`. - - Examples of not falling under domain scope: `App\Models\CustomAppBaseModel`, `Illuminate\Database\Eloquent\NonExistentModel`. + - If the configured `ddd.base_model` exists (evaluated using `class_exists`), base model generation is skipped. + - If `ddd.base_model` does not exist and falls under a domain namespace, base model will be generated. + - Falling under a domain namespace means `Domain\**\Models\SomeBaseModel`. + - For example, if `ddd.base_model` were set to `App\Models\CustomAppBaseModel` or `Illuminate\Database\Eloquent\NonExistentModel`, they fall outside of the domain namespace and won't be generated on your behalf. ### Fixed -- Resolve long-standing issue where `ddd:model` would not properly detect whether the configured basemodel in `ddd.base_model` already exists, leading to unpredictable results when `ddd.base_model` deviated from the default `Domain\Shared\Models\BaseModel`. +- Resolve long-standing issue where `ddd:model` would not properly detect whether the configured `ddd.base_model` already exists, leading to unpredictable results when `ddd.base_model` deviated from the default `Domain\Shared\Models\BaseModel`. + +### Chore +- Update composer dependencies. ## [0.7.0] - 2023-10-22 ### Added diff --git a/composer.json b/composer.json index 76d4256..49a48f4 100644 --- a/composer.json +++ b/composer.json @@ -45,6 +45,7 @@ "Lunarstorm\\LaravelDDD\\Tests\\": "tests", "App\\": "vendor/orchestra/testbench-core/laravel/app/", "Database\\Factories\\": "vendor/orchestra/testbench-core/laravel/database/factories/", + "Database\\Seeders\\": "vendor/orchestra/testbench-core/laravel/database/seeders/", "Domain\\": "vendor/orchestra/testbench-core/laravel/src/Domain/" } }, diff --git a/src/Commands/MakeModel.php b/src/Commands/MakeModel.php index 72d649e..3239bfe 100644 --- a/src/Commands/MakeModel.php +++ b/src/Commands/MakeModel.php @@ -56,12 +56,13 @@ protected function preparePlaceholders(): array return [ 'extends' => filled($baseClass) ? " extends {$baseClassName}" : '', + 'baseClassImport' => filled($baseClass) ? "use {$baseClass};" : '', ]; } public function handle() { - $this->createBaseModelIfNeeded(config('ddd.base_model')); + $this->createBaseModelIfNeeded(); parent::handle(); @@ -70,8 +71,10 @@ public function handle() } } - protected function createBaseModelIfNeeded($baseModel) + protected function createBaseModelIfNeeded() { + $baseModel = config('ddd.base_model'); + if (class_exists($baseModel)) { return; } @@ -79,16 +82,16 @@ protected function createBaseModelIfNeeded($baseModel) $this->warn("Configured base model {$baseModel} doesn't exist."); // If the base model is out of scope, we won't attempt to create it - // because we don't want to interfere with outside folders. - $allowedRootNamespaces = [ + // because we don't want to interfere with external folders. + $allowedNamespacePrefixes = [ $this->rootNamespace(), ]; - if (! str($baseModel)->startsWith($allowedRootNamespaces)) { + if (! str($baseModel)->startsWith($allowedNamespacePrefixes)) { return; } - $domain = DomainResolver::fromClass($baseModel); + $domain = DomainResolver::guessDomainFromClass($baseModel); if (! $domain) { return; @@ -101,7 +104,7 @@ protected function createBaseModelIfNeeded($baseModel) $this->info("Generating {$baseModel}..."); $this->call(MakeBaseModel::class, [ - 'domain' => $domain->domain, + 'domain' => $domain, 'name' => $baseModelName, ]); } diff --git a/src/Support/DomainResolver.php b/src/Support/DomainResolver.php index 432d6ec..04efe97 100644 --- a/src/Support/DomainResolver.php +++ b/src/Support/DomainResolver.php @@ -2,15 +2,13 @@ namespace Lunarstorm\LaravelDDD\Support; -use Illuminate\Support\Str; - class DomainResolver { - public static function fromClass(string $class): ?Domain + public static function guessDomainFromClass(string $class): ?string { $domainNamespace = basename(config('ddd.paths.domains')).'\\'; - if (! Str::startsWith($class, $domainNamespace)) { + if (! str($class)->startsWith($domainNamespace)) { // Not a domain model return null; } @@ -20,6 +18,6 @@ public static function fromClass(string $class): ?Domain ->before('\\') ->toString(); - return new Domain($domain); + return $domain; } } diff --git a/stubs/model.php.stub b/stubs/model.php.stub index f8ff6e8..e775c33 100644 --- a/stubs/model.php.stub +++ b/stubs/model.php.stub @@ -2,7 +2,7 @@ namespace {{ namespace }}; -use {{ rootNamespace }}\Shared\Models\BaseModel; +{{ baseClassImport }} use Illuminate\Database\Eloquent\SoftDeletes; class {{ class }}{{ extends }} diff --git a/tests/Generator/MakeModelTest.php b/tests/Generator/MakeModelTest.php index 0028eec..5884d66 100644 --- a/tests/Generator/MakeModelTest.php +++ b/tests/Generator/MakeModelTest.php @@ -119,6 +119,13 @@ "{$modelName}.php", ])); + $expectedModelClass = implode('\\', [ + basename(config('ddd.paths.domains')), + $domain, + config('ddd.namespaces.models'), + $modelName, + ]); + if (file_exists($expectedModelPath)) { unlink($expectedModelPath); } @@ -131,17 +138,23 @@ unlink($expectedBaseModelPath); } + expect(class_exists($baseModelClass))->toBeFalse(); + expect(file_exists($expectedBaseModelPath))->toBeFalse("{$baseModelPath} expected not to exist."); Artisan::call("ddd:model {$domain} {$modelName}"); - expect(file_exists($expectedBaseModelPath))->toBeTrue("{$baseModelPath} expected to exist."); + expect(file_exists($expectedBaseModelPath))->toBeTrue("Expecting base model file to be generated at {$baseModelPath}"); + + // Not able to properly assert the following class_exists checks under the testing environment + // expect(class_exists($expectedModelClass))->toBeTrue("Expecting model class {$expectedModelClass} to exist"); + // expect(class_exists($baseModelClass))->toBeTrue("Expecting base model class {$baseModelClass} to exist"); })->with([ - ['Domain\\Shared\\Models\\BaseModel', 'src/Domain/Shared/Models/BaseModel.php'], - ['Domain\\Infrastructure\\Models\\BaseModel', 'src/Domain/Infrastructure/Models/BaseModel.php'], + ['Domain\Shared\Models\CustomBaseModel', 'src/Domain/Shared/Models/CustomBaseModel.php'], + ['Domain\Core\Models\CustomBaseModel', 'src/Domain/Core/Models/CustomBaseModel.php'], ]); -it('will not create a base model if the configured base model is out of scope', function ($baseModel) { +it('will not generate a base model if the configured base model is out of scope', function ($baseModel) { Config::set('ddd.base_model', $baseModel); expect(class_exists($baseModel))->toBeFalse(); @@ -151,9 +164,11 @@ expect(Artisan::output()) ->toContain("Configured base model {$baseModel} doesn't exist.") ->not->toContain("Generating {$baseModel}"); + + expect(class_exists($baseModel))->toBeFalse(); })->with([ - ['Illuminate\\Database\\Eloquent\\NonExistentModel'], - ['OtherVendor\\OtherPackage\\Models\\OtherModel'], + ['Illuminate\Database\Eloquent\NonExistentModel'], + ['OtherVendor\OtherPackage\Models\NonExistentModel'], ]); it('skips base model creation if configured base model already exists', function ($baseModel) { @@ -167,8 +182,8 @@ ->not->toContain("Configured base model {$baseModel} doesn't exist.") ->not->toContain("Generating {$baseModel}"); })->with([ - ['Illuminate\\Database\\Eloquent\\Model'], - ['Lunarstorm\\LaravelDDD\\Models\\DomainModel'], + ['Illuminate\Database\Eloquent\Model'], + ['Lunarstorm\LaravelDDD\Models\DomainModel'], ]); it('shows meaningful hints when prompting for missing input', function () { diff --git a/tests/Model/FactoryTest.php b/tests/Model/FactoryTest.php index 9b33e11..afe9368 100644 --- a/tests/Model/FactoryTest.php +++ b/tests/Model/FactoryTest.php @@ -2,6 +2,7 @@ use Illuminate\Database\Eloquent\Factories\Factory; use Illuminate\Support\Facades\Artisan; +use Illuminate\Support\Facades\Config; use Lunarstorm\LaravelDDD\Factories\DomainFactory; it('can resolve the factory name of a domain model', function ($modelClass, $expectedFactoryClass) { @@ -13,6 +14,7 @@ ]); it('can instantiate a domain model factory', function ($domainParameter, $modelName, $modelClass) { + Config::set('ddd.base_model', 'Lunarstorm\LaravelDDD\Models\DomainModel'); Artisan::call("ddd:model -f {$domainParameter} {$modelName}"); expect(class_exists($modelClass))->toBeTrue(); expect($modelClass::factory())->toBeInstanceOf(Factory::class); diff --git a/tests/TestCase.php b/tests/TestCase.php index 44aaa10..9788b7f 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -22,6 +22,7 @@ protected function setUp(): void // Reset the domain namespace Arr::forget($data, ['autoload', 'psr-4', 'Domains\\']); + Arr::forget($data, ['autoload', 'psr-4', 'Domain\\']); // Set up the essential app namespaces data_set($data, ['autoload', 'psr-4', 'App\\'], 'vendor/orchestra/testbench-core/laravel/app'); @@ -36,7 +37,9 @@ protected function setUp(): void fn (string $modelName) => 'Lunarstorm\\LaravelDDD\\Database\\Factories\\'.class_basename($modelName).'Factory' ); - $this->beforeApplicationDestroyed(fn () => $this->cleanFilesAndFolders()); + $this->beforeApplicationDestroyed(function () { + $this->cleanFilesAndFolders(); + }); } protected function getPackageProviders($app)