refactor new module structure, without namespace app
#1758
Conversation
|
thanks @alissn, this looks ideal. I'm having an issue, can you describe what steps need to be done for this to work? I've loaded this PR locally into a project deleted the existing config and published the updated version from this PR. I've left the root composer.json as is so existing modules are loaded as normal. I've then generated a new module called Contacts which results in:
the generated modules composer file only contains: oooh got it, I needed to add the merge to the root composer file: |
|
Any way to solve this, in PhpStorm it complains the file is not in the correct path 
a few things I've noticed generating a test goes into the root of the module probably due to the autoloading issue illustrated above. The route service provider contains a hardcoded app path
so if its possible to resolve the autoloading issue above I think everything else should work or is a simple path update. |
|
@dcblogdev Because the module service provider is loaded from the root of the project, after creating a new module, we should run I've added a section to run after creating the module, but it seems not to be working properly. 🥲 Please run the command composer dump-autoload or composer update manually. i think resolve the issue. |
|
@dcblogdev However, I did not encounter the warning in my PHPStorm🤔. My PHPStorm version is 2024.1 beta (EAP). 
|
|
The module does run but composer does complain about the path: 
The modules does load in the browser. |
|
@dcblogdev diff --git a/composer.json b/composer.json
index 318af3c..ca39b6e 100644
--- a/composer.json
+++ b/composer.json
@@ -14,7 +14,7 @@
"laravel/framework": "^10.10",
"laravel/sanctum": "^3.3",
"laravel/tinker": "^2.8",
- "nwidart/laravel-modules": "^10.0"
+ "nwidart/laravel-modules": "dev-UpdateAppFolder"
},
"require-dev": {
"fakerphp/faker": "^1.9.1",
@@ -30,7 +30,6 @@
"autoload": {
"psr-4": {
"App\\": "app/",
- "Modules\\": "Modules/",
"Database\\Factories\\": "database/factories/",
"Database\\Seeders\\": "database/seeders/"
}
@@ -58,6 +57,11 @@
"extra": {
"laravel": {
"dont-discover": []
+ },
+ "merge-plugin": {
+ "include": [
+ "Modules/*/composer.json"
+ ]
}
},
"config": {
@@ -66,9 +70,16 @@
"sort-packages": true,
"allow-plugins": {
"pestphp/pest-plugin": true,
- "php-http/discovery": true
+ "php-http/discovery": true,
+ "wikimedia/composer-merge-plugin": true
}
},
"minimum-stability": "stable",
- "prefer-stable": true
-}
\ No newline at end of file
+ "prefer-stable": true,
+ "repositories": [
+ {
+ "type": "path",
+ "url": "./../laravel-modules"
+ }
+ ]
+}i think forget delete |
|
thanks using the breeze demo I'm not getting the warning now for new modules but the existing modules load fine but composer shows when doing composer dump-autoload: 
breaking this down further its cased because both app and App work causing 2 paths to exist same for other folders that have changed such as database. The raw version: |
|
This warning occurs because the old version of this package autoloads all folders under some namespace: This causes one class to have two namespaces. To fix it, the change should be like this: diff --git a/Modules/Profile/composer.json b/Modules/Profile/composer.json
index 963a4ac..a736180 100644
--- a/Modules/Profile/composer.json
+++ b/Modules/Profile/composer.json
@@ -17,8 +17,7 @@
},
"autoload": {
"psr-4": {
- "Modules\\Profile\\": "",
- "Modules\\Profile\\App\\": "app/",
+ "Modules\\Profile\\": "app/",
"Modules\\Profile\\Database\\Factories\\": "database/factories/",
"Modules\\Profile\\Database\\Seeders\\": "database/seeders/"
}
(END) |
|
I've uploaded my test project to this repository: UpdateNamespace |
|
After a bit of trial and error I think this should be good to go, I wanted to ensure that once merged existing modules won't be broken and updating is minimal. So for existing modules that use capital folder convention like becomes Generating tests files is being created in the root and not the correct folder structure. I think we need to test this a fair bit more before its ready. I'll do a release for v11 with a regular structure then this can be added later. |
|
My idea is to delay the release to ensure compatibility with Laravel 11. This release will be a major version and may contain breaking changes. Releasing a new version will make it difficult to change certain things. Based on the Laravel version-based folder structure below, all folders excluding The new module generation structure will be similar to this: This structure is similar to the Laravel structure. Additionally, the namespace of the factory and seeder is defined in the root "autoload": {
"psr-4": {
"App\\": "app/",
"Database\\Factories\\": "database/factories/",
"Database\\Seeders\\": "database/seeders/"
}
},
"autoload-dev": {
"psr-4": {
"Tests\\": "tests/"
}
},For the new generated class, it will look similar to this: "autoload": {
"psr-4": {
"Modules\\Ali\\": "app/",
"Modules\\Ali\\Database\\Factories\\": "database/factories/",
"Modules\\Ali\\Database\\Seeders\\": "database/seeders/"
}
},
"autoload-dev": {
"psr-4": {
"Modules\\Ali\\Tests\\": "tests/"
}
} |
|
Yep, that structure looks good, my only worry is not making it too difficult to update existing projects that may have 50+ modules. But it does look like there will need to be some changes made to existing modules. |
|
thank you, I'll take a closer look at this tomorrow. |
|
I like how this is going. good work and thanks. |
|
I've identified an issue with the tests. The test configuration is hardcoded and does not fully cover the package functionality. fix on this pull request #1764. After merging the three pull requests, I'm confident that everything works very well. |
No. 1 is a great idea. I recommended the same feature a few months ago but couldn't defend it to the community's satisfaction. However, is removing the App\ from the namespace in compliance with Laravel 11.* or something else? Where is this concept coming from? PSR-0/PSR-4, Laravel, PHP itself, etc.? We should try to keep the structure and namespace close to the original Laravel framework as much as possible. Code reusability and maintainability is the foundation of modularity. Logically speaking, I don't see the need for No. 2 to 6, suggesting the removal of the App\ from the namespace. Modules should be friendly for those already comfortable with the standard Laravel structure and also these moving things back from Modules to the core framework. The App\ should be retained, except if Laravel, PSR, etc. have recommended removing it from the standard structure too. We are breaking down the original Laravel framework into a smaller unit, not building a custom framework! If I may ask, what is the motive behind removing the App\ fragment from the namespace? |
|
😆 calm down. It is just like you said. |
I was referring to the namespace, sorry for the typo. PSR-4 is also something we should keep up with. |
|
It is psr-4 compatible. Don't worry. |
It's not! Namespaces are generated in compliance with the path structure. Most modifications are to alter the case (e.g. |
It's the standard structure of Laravel core, as you can see from laravel/laravel. |
|
@alissn one issue I've come across with the new structure is loading Livewire components (not full page, they work fine) We use an external package to auto-register Livewire classes that load from a "Livewire" namespace located in the root of the module. For layout components that are embedded, they need to be registered manually otherwise they cannot be found. I've been converting over my website to use the new structure 
I have a search component that's loaded via I may need to raise an issue with the package, maybe something that could be addressed from there. Or leave Livewire in the root. Also, we should add a test autoload into the generated composer: |
|
Now it uses composer merge plugin. And Its (Modules namespace) not defined by the root composer.json. Instead every composer.json in modules directory has its own setup which directs Modules\Blog to \app folder for example if you look at the examples you can have deeper understanding. That's why it never modifies app to App or removes anything. Just like as @alissn mentioned, the main laravel repo use it in this way and with the same structure. |
From the link you shared, you can see the app/ boldly in place. Over customization will deviate the purpose of this package. Many of us have custom designs, but we keep it to our system if it's not something that will meet standards. |
I've fixed the issue with layout component class in my previous PR. For the tests autoloader, there is already a provision for it in the module composer.json, nevertheless you'll need to update the PHP unit XML file at the root. Include Modules/*/Tests |
I'm very certain you don't have clue of what we are discussing here. Scroll up a little and reference the link shared by @alissn and you'll see for yourself. All I'm saying is we should separate personal perceptions and conversations from standardization. Let's keep the codebase clean and clear for everyone. Nothing personal!!! |
|
|
Do as you like. |
It's not about me, it's for the community! Don't take it personal!! |
|
All I say is, it doesn't have to match. All libraries out there works like this.
I am not going to continue to mess up this conversation. |
"autoload": {
"psr-4": {
"Modules\\Base\\App\\": "app/",
"Modules\\Base\\Database\\Factories\\": "database/factories/",
"Modules\\Base\\Database\\Seeders\\": "database/seeders/"
}
}Long before now, I've been using this wiki-media package in my dev. I've even submitted a PR regarding it in the past. Please, let's put everyone interest in mind! |
There is a difference between development tools and system tools. How can you be comparing them? Development tool like this package is a skeleton to develop another apps (Modules), system tools like Livewire you gave is to be extended only. You don't rewrite Livewire, it's a core package. I don't know how to explain this to your understanding. Laravel-modules is a package, but the generated modules are apps. There are not the same. Within the laravel-modules package, they can use any customizations they want, but not with the generated modules. The generated Modules should follow the underlying structures, concept and standards - Laravel. |
|
However, we can unmerge/undo this PR as many others and other commits has been updated. I'll recommend another PR to correct the namespace alterations and the addition of the app_folder which services no purpose. However, many mistakes were done from the onset. I'll encourage the contributors to keep close to the underlying framework (Laravel) for constant updates. Our aims should be in breaking the base structure down to modules. If there is a custom features, we should make it a standalone or at least a command. In that case people will choose to use the command or package when needed! |
|
Hi Armin, This pull request only fixes the old structure. If you delete 
For example, the
Is this okay for you? This is the only breaking change with new modules created with the You can see our company project structure without the The only breaking change of this pull request is in the root Delete this line: - "Modules\\": "Modules/",and add this section: + },
+ "merge-plugin": {
+ "include": [
+ "Modules/*/composer.json"
+ ]
} |
|
thank you, everyone, it's important we have these discussions. From my tests, I've been able to use the new structure for new modules but also keep an existing structure that doesn't use an app folder at all. It's important that anyone upgrading has minimal changes not forcing updating multiple modules. Also, it's worth noting the structure in the config would only affect new modules and you can fully change this for your projects as desired. If you're happy having all folders capitalised then you 100% can do that. |
To fix it, I created two pull requests. After that, everything works very well:
|
|
Thank you everyone, we have done more than enough justice to this PR and I feel is time to move on. We can't reverse this PR. The removal of the App/ section in the namespace is our major concern, which I believe we can simply create a new PR to correct it. Other than that, I must confess this PR is amazing. Again, @dcblogdev please be careful when merging PR to be sure it's not reversing an issue already solved. This issue of module structure and namespace was already fixed by myself in the past. Little after that I found that another PR was merged that took us back to the issue again. When I try to resolve the issue with @dcblogdev I couldn't, so I just ignored it. We should always review and consider the benefits/effects a PR will have on the community. I appreciate all my fellow contributors, please keep up the great work. |
Hi,
I understand that there are many files changed, making review challenging. My apologies for this🙏.
This PR comprises several changes, outlined below:
1. Adding Package for Discovery
Added package
wikimedia/composer-merge-pluginhere to the core of this package. This addition allows us to eliminate the need to defineModulewithin the base Laravel composer file. Instead, we define the paths of our modules as follows:This enables automatic discovery of module composer.json files, hence their providers.
2. Defining Config
app_folderDefined the configuration path of the
appfolder (on InterNACHI/modular assrc). This configuration aids in removing the folder name from the generated namespace.3. Updating Stub of compser.json
New PSR-4 namespace generation is as follows:
"autoload": { "psr-4": { - "Modules\\Base\\": "", - "Modules\\Base\\App\\": "app/", + "Modules\\Base\\": "app/", "Modules\\Base\\Database\\Factories\\": "database/factories/", "Modules\\Base\\Database\\Seeders\\": "database/seeders/" } },Other folders outside
appdo not require a namespace.4. Sorting Generator Config
Sorted generator files based on folder type (
app,database,route,resource, etc.) for improved readability and easier navigation.5. Updating
getDefaultNamespaceon Make CommandsThis method generates namespaces based on file paths, removing the
app_foldername before generating the namespace.Namespace change example:
6. Minor Changes to Pass Tests
Updated some stubs and snapshots to ensure passing tests.
todos :