lunarstorm/laravel-ddd

Autoloading issues

Closed this issue · 9 comments

pelmered commented

Hello again!

I'm having some issues with the autoloading. Primarily when deploying the code.

I like to put my related tests inside my domains (a command for generating tests would be great by the way), but that generates problems with the autoloading. The tests are only autoloaded when composer includes dev dependencies. But since theddd:cache command touches all files in the domain directory, this generates problems because the base test case is not autoloaded and thus I get this error: Class "Tests\TestCase" not found when running ddd:cache.

I created a repo where I demonstrate this problem here: https://github.com/pelmered/laravel-test-ddd
Just clone, run composer install --no-dev and then run ddd:cache to reproduce the problem.

I have also had problems with some other things, for example the migrations from babenkoivan/elastic-migrations. That package creates migrations with the same file format as the default Laravel migrations, but they also include a class name so they are not PSR autoload compliant. Here is how they look. When autoloading or running ddd:cache these files are included several times and therefore gives this kind of error: Cannot declare class CreateUsersIndex, because the name is already in use. I created a PR there to use anonymous classes, just like Laravels migrations, here.

As you can see, this is a general problem that probably needs to be addressed in this package in some way.
What do you think is the best way forward? Let me know if I can be of assistance.

Other than this, I'm happy to report that it worked very well in our application and that the transition was very smooth to start using the autoload provided in this package.

JasperTey commented

Hello again!

I'm having some issues with the autoloading. Primarily when deploying the code.

I like to put my related tests inside my domains (a command for generating tests would be great by the way), but that generates problems with the autoloading. The tests are only autoloaded when composer includes dev dependencies. But since theddd:cache command touches all files in the domain directory, this generates problems because the base test case is not autoloaded and thus I get this error: Class "Tests\TestCase" not found when running ddd:cache.

I created a repo where I demonstrate this problem here: https://github.com/pelmered/laravel-test-ddd Just clone, run composer install --no-dev and then run ddd:cache to reproduce the problem.

Ah, so is it a matter where ddd autoloading should be ignoring the test directory if there is one? And therefore be able to configure a designated domain test folder to ignore. Or more generally, maybe a configurable ddd.autoload.ignore.

I have also had problems with some other things, for example the migrations from babenkoivan/elastic-migrations. That package creates migrations with the same file format as the default Laravel migrations, but they also include a class name so they are not PSR autoload compliant. Here is how they look. When autoloading or running ddd:cache these files are included several times and therefore gives this kind of error: Cannot declare class CreateUsersIndex, because the name is already in use. I created a PR there to use anonymous classes, just like Laravels migrations, here.

As you can see, this is a general problem that probably needs to be addressed in this package in some way. What do you think is the best way forward? Let me know if I can be of assistance.

I see, I remember this kind of behaviour back when Laravel wasn't using anonymous classes yet for migrations. That being said, are you generating migrations inside the domain layer then, hence why it gets picked up by ddd:cache?

Other than this, I'm happy to report that it worked very well in our application and that the transition was very smooth to start using the autoload provided in this package.

pelmered commented

Ah, so is it a matter where ddd autoloading should be ignoring the test directory if there is one? And therefore be able to configure a designated domain test folder to ignore. Or more generally, maybe a configurable ddd.autoload.ignore.

Yes, some kind of general solution like an configurable ignore would be great. An array with paths inside each Domain(relative to domain root) that should be ignored would work. Something like this:

'ignore' => [
    'Tests',
    'Database/ElasticMigrations',
    'Any/Other/Folder/To/Ignore',
],

I did not see any support for this in Lody, but it should be possible to do this with a custom finder filter.

I see, I remember this kind of behaviour back when Laravel wasn't using anonymous classes yet for migrations. That being said, are you generating migrations inside the domain layer then, hence why it gets picked up by ddd:cache?

Yes, I like to put migrations, factories and seeders inside the domain they belong to. The ignore could be used to solve this as well 👍

JasperTey commented

I wonder if ddd:migration and ddd:test should be added at some point. In your case, you're only generating domain migrations by way of the elastic-migrations package, I take it?

Being able to bundle in tests and migrations would make the domain fully portable and copy-paste-able from one app to another at that point.

pelmered commented

I wonder if ddd:migration and ddd:test should be added at some point.

That would be great.
Even better of you could also add the flags for generating factories, migrations, tests, policies etc. to the ddd:model command. Like the default make:model command has. That was a function I missed yesterday when I made my first model using this package.

In your case, you're only generating domain migrations by way of the elastic-migrations package, I take it?

Yes, that package provides an elastic:make:migration command that I use.

Being able to bundle in tests and migrations would make the domain fully portable and copy-paste-able from one app to another at that point.

I'm working on a product company with a single Laravel project so that is not my primary reason, but if you are working with multiple projects I could imagine that would be would be very handy.

JasperTey commented

Yeah, the portable-domain scenario is well suited for those who build multiple apps for various clients (such as me).

I will work on these updates over the weekend.

pelmered commented

Yeah, the portable-domain scenario is well suited for those who build multiple apps for various clients (such as me).

I will work on these updates over the weekend.

Great, thanks a lot!

I was able to filter out the tests using this code:

        return Lody::classesFromFinder(
            Finder::create()
                  ->files()
                  ->in($paths)
                  ->filter(function ($file) {
                      if (Str::startsWith(strstr($file->getRelativePathname(), '/'), '/Tests/')) {
                          return false;
                      }
                  }))
            ->isNotAbstract()
            ->isInstanceOf(Command::class)
            ->toArray();

So, something like that would work.

Another idea that I think would be great is support for something like this to provide your own filter callback like this in your service provider:

DDD::filterPathsUsing(function ($file) {
    // Your custom callback
});

Then you could implement your own logic for this and not just and array of simple startsWith strings.

JasperTey commented

I created a repo where I demonstrate this problem here: https://github.com/pelmered/laravel-test-ddd
Just clone, run composer install --no-dev and then run ddd:cache to reproduce the problem.

I cloned the repo and ran composer install --no-dev, which yielded the following:

image

Everything works if I do a composer install without --no-dev, but then ddd:cache works and doesn't produce any error. What am I missing? EDIT: I was able to eventually replicate the scenario. A subsequent composer install --no-dev AFTER an initial composer install seemed to do it. vs composer install --no-dev immediately from a fresh clone. Don't really understand why.

JasperTey commented

...however, I was able to simulate the conflicting migration classes scenario through the package tests, so I can move forward with an autoload.ignore implementation.

JasperTey commented

I've been tinkering with this, and some challenges I'm facing at the moment:

For the non-psr migrations residing inside the domain issue, if I understand correctly, this is a violation at the composer level, since everything under "Domain\\" in composer.json's autoload psr-4 is expected to be valid psr-4. This package wouldn't be able to intervene since the error is thrown by composer during the post-autoload-dump hook. I looked into https://getcomposer.org/doc/04-schema.md#exclude-files-from-classmaps - but haven't had success with it yet. It makes sense now why laravel apps store migrations outside of the psr-4 App\\ namespace.

For the autoload.ignore behaviour, the snippet above applying a ->filter() to exclude /Tests/ is a good start, I need to spend more time generalizing it and ensuring it can support subdomains (which always complicates things).