Update config.php

[solomon-ochepa]

Format the file structure to the current Laravel standard.

These changes will only apply to newly created modules.

[solomon-ochepa]

Please, I need an explanation as to why this PR was closed without being merged.

This particular PR is crucial, and I expected it to be treated before any other. Please quote me if I'm wrong!

[dcblogdev]

this is making sweeping changes that I don't want. This would require a version dump due to breaking changes.

In future, before making fundamental changes I suggest asking to see if it would be well received to save your time.

You're free to change the config to your liking in your application. I try to keep changes in the config to essential changes only to avoid breaking existing applications that may not have published the config file.

[solomon-ochepa]

this is making sweeping changes that I don't want. This would require a version dump due to breaking changes.

In future, before making fundamental changes I suggest asking to see if it would be well received to save your time.

You're free to change the config to your liking in your application. I try to keep changes in the config to essential changes only to avoid breaking existing applications that may not have published the config file.

You are right, but not in all aspects.

'statuses-file' => base_path('modules.json'),

There is a breaking change at line: 274, which is true (sorry, my mistake).

However, the rest of the code is as clean as possible, and wouldn't cause any form of breaking changes from Laravel 5 up to date.

Please, do point out any of the other observations.

Thanks.

[solomon-ochepa]

Please, can you reopen the PR so I can make an edit so we can continue the review?

[dcblogdev]

You're right the generator changes don't break existing modules. I don't think its a good idea to be a mix of namespaces names ie:

a controller would have lower case app

namespace Modules\Contacts\app\Http\Controllers;

or a seeder

namespace Modules\Contacts\database\seeders;

They would be better to match the style ie

namespace Modules\Contacts\App\Http\Controllers;
namespace Modules\Contacts\Database\Seeders;

[solomon-ochepa]

They would be better to match the style ie

namespace Modules\Contacts\App\Http\Controllers;
namespace Modules\Contacts\Database\Seeders;

That's not correct, following the Laravel code standard.

Note that the namespace should be assigned in accordance with the path! Note that the /app and /database/seeders directories are in lower case.

[dcblogdev]

I disagree, for instance, looking at seeders from Laravel their namespace is Database\Seeders not database\seeders

[solomon-ochepa]

I disagree, for instance, looking at seeders from Laravel their namespace is Database\Seeders not database\seeders

That's true. It's configured via the composer autoload: psr-4 rule. Laravel modules on the other side only loaded the Modules/ path.

Can you please review the standard Laravel's composer.json?

    "autoload": {
        "psr-4": {
            "App\\": "app/",
            "Database\\Factories\\": "database/factories/",
            "Database\\Seeders\\": "database/seeders/",
            "Modules\\": "Modules/"
        }
    },

Notwithstanding, I think your concept may be possible if we added these same rules to each generated module's composer.json file.

Moreover, in a future release, you may consider renaming the root path from /Modules to /modules.

Thanks.

[solomon-ochepa]

Hi, @dcblogdev.

I'm still waiting for you here, please.

[dcblogdev]

if you can get this to work with the namespaces Modules\Books\Http\Controllers etc we can move forward.

I've tried the config and adding

"psr-4": {
    "Modules\\Books\\": "app/",
    "Modules\\Books\\Tests\\": "tests",
    "Modules\\Books\\Database\\Factories\\": "database/factories",
    "Modules\\Books\\Database\\Seeders\\": "database/seeders"
}

but having problems getting this to work.

[solomon-ochepa]

If you can get this to work with the namespaces Modules\Books\Http\Controllers etc we can move forward.

I've tried the config and adding

"psr-4": {
    "Modules\\Books\\": "app/",
    "Modules\\Books\\Tests\\": "tests",
    "Modules\\Books\\Database\\Factories\\": "database/factories",
    "Modules\\Books\\Database\\Seeders\\": "database/seeders"
}

but having problems getting this to work.

You are complicating the whole process! This PR has nothing to do with the composer.json file within each module, as illustrated in your example above.

This PR aims to give each new module the same file structure as the latest Laravel version.

From Laravel v5.* to the current v10.*, the Controller's namespace, for instance, has been app/Http/Controllers.

So, if a new module Book is to be created using the latest Laravel version, it would be Modules\Books\ app\Http\Controllers, instead of Modules\Books\ Http\Controllers as given in your example above.

Making the app in the namespace appear as App is not the aim of this PR in the first place, so where is your argument coming from?

We can work on making the app appear as App in subsequent PR, but not within the scope of this particular PR, in the sense that the /composer.json responsible for this PR effect is entirely different from the /Modules/Books/composer.json, responsible for the result you are seeking. Do you get my point now?

[solomon-ochepa]

...
I've tried the config and adding

"psr-4": {
    "Modules\\Books\\": "app/",
    "Modules\\Books\\Tests\\": "tests",
    "Modules\\Books\\Database\\Factories\\": "database/factories",
    "Modules\\Books\\Database\\Seeders\\": "database/seeders"
}

but having problems getting this to work.

Please take note of the "Modules\\Books\\": "app/" it should be "Modules\\Books\\App": "app/" instead.

Try this out instead:

"psr-4": {
    "Modules\\Books\\": "",
    "Modules\\Books\\App\\": "app/",
    "Modules\\Books\\Tests\\": "tests/",
    "Modules\\Books\\Database\\Factories\\": "database/factories/",
    "Modules\\Books\\Database\\Seeders\\": "database/seeders/"
}

[solomon-ochepa]

The namespace can be changed from the config file.

Notwithstanding, I'm working on how to get the value directly from the module's composer.json file.

[mstojanovv]

This change has resulted in a less appealing namespace structure, in fact, not really following Laravel modern architecture or standards.

The usage of lowercase directory namespace like Modules\ModuleName\app\Providers, Modules\ModuleName\database\seeders, affects the overall code.

I believe that @dcblogdev or @solomon-ochepa should have addressed the issue of these lowercase directory related namespace, including app, database, and any others located at the root of the module, before merging the change into the package.

The PR is great, but using a lowercase namespace folder is not that much.

Despite my attempts to find various solutions, I haven't found any solution to make it working.

[solomon-ochepa]

This change has resulted in a less appealing namespace structure, in fact, not really following Laravel modern architecture or standards.

The usage of lowercase directory namespace like Modules\ModuleName\app\Providers, Modules\ModuleName\database\seeders, affects the overall code.

I believe that @dcblogdev or @solomon-ochepa should have addressed the issue of these lowercase directory related namespace, including app, database, and any others located at the root of the module, before merging the change into the package.

The PR is great, but using a lowercase namespace folder is not that much.

Despite my attempts to find various solutions, I haven't found any solution to make it working.

Point of correction, the structures are accurate and follows Laravel current standard.

The only point we are currently missing and seeking solution to is how to map the namespaces.

Like in Laravel, the app/, database/migrations/ & database/seeders namespaces are not based on the paths lowercase names, but rather were config via the composer.json file.

We are currently working towards that, in short we have added the necessary properties to the Module's composer.json already.

So, please, this issue has nothing to do with the lowercase directories.