Fix issues with app path and namespace

[solomon-ochepa]

Summary
Resolve issues with the app/ path by supporting both default and custom names while ensuring proper namespace conversion.

Description
The app/ path was previously removed due to multiple issues. This task involves reintroducing support for it while ensuring:

• It correctly supports both default (app/) and custom paths (src/, /).
• The namespace is properly converted to StudlyCase.
• No existing features are broken in the process.
• Tests have been added to verify correctness and stability.

Acceptance Criteria:

• [x] app/ path works with both default and custom configurations.
• [x] Namespace is correctly converted to StudlyCase.
• [x] No feature breakages in this branch.
• [x] Tests validate the expected behavior.

Tests (PathNamespaceTest)

• test_generates_app_path()
• test_generates_custom_app_path()
• test_generates_root_app_path()
• test_removes_duplicate_app_path()
• test_identifies_app_path()
• test_recognizes_custom_app_path(), etc.

Let me know if you want any modifications! 🚀

[solomon-ochepa]

Hi, @dcblogdev. Your attention is needed here, please.

[dcblogdev]

this needs to work with the existing app_folder path in the config otherwise it will break for existing apps.

Confirmed app and src folder work as long as the modules composer are updated accordingly such as "Modules\\Blog\\": "app/",

[solomon-ochepa]

this needs to work with the existing app_folder path in the config otherwise it will break for existing apps.

That is true. I'll immediately add a backward compatibility replacement for the old app_folder.

Confirmed app and src folder work as long as the modules composer are updated accordingly such as "Modules\\Blog\\": "app/",

Sure, they are all working, and I've written some tests for that. You can go ahead and confirm it.

Tests:

test_generates_app_path() (modules.paths.app: 'app/' or '') - default path.

public function test_generates_app_path()
    {
        $app_path = $this->path_namespace->app_path();

        $this->assertSame('app/Models/User', $this->path_namespace->app_path('Models/User'));
        $this->assertSame($app_path, $this->path_namespace->app_path(null));
    }

test_generates_custom_app_path() (modules.paths.app: 'src/') - any custom path of your choice, including src/

public function test_generates_custom_app_path()
    {
        config(['modules.paths.app' => 'src/']);

        $this->assertSame('src/Models/User', $this->path_namespace->app_path('Models/User'));
    }

test_generates_root_app_path() (modules.paths.app: '/') - can also be used without the app/ path

public function test_generates_root_app_path()
    {
        config(['modules.paths.app' => '/']);

        $this->assertSame('Models/User', $this->path_namespace->app_path('Models/User'));
    }

[solomon-ochepa]

Sorry, your merge affected the workflow. I'll be submitting a new PR shortly. @dcblogdev

[solomon-ochepa]

this needs to work with the existing app_folder path in the config otherwise it will break for existing apps.

Confirmed app and src folder work as long as the modules composer are updated accordingly such as "Modules\\Blog\\": "app/",

Hi, @dcblogdev.

The backward compatibility support for both modules.paths.app_folder and composer.json $APP_FOLDER_NAME$ has been added successfully. Please check it out and let me know if more changes are needed.

Thanks, and have a nice day.

[dcblogdev]

thanks, I'll have a look at this tonight.

[alissn]

After merging this PR, we’ll likely be saying hi to a lot of new issues. 🤷‍♂️

Having 230 changed files is not normal. It would be better if the author could split this into smaller, more focused PRs so the community can review the changes more effectively.

[solomon-ochepa]

After merging this PR, we’ll likely be saying hi to a lot of new issues. 🤷‍♂️

Having 230 changed files is not normal. It would be better if the author could split this into smaller, more focused PRs so the community can review the changes more effectively.

Do you discover any bug you'd like the author to correct?

The CI/CD tests was created to detect early bugs. If passed, the code is believed to be okay!

Thanks and have a great day

[solomon-ochepa]

After merging this PR, we’ll likely be saying hi to a lot of new issues. 🤷‍♂️

Having 230 changed files is not normal. It would be better if the author could split this into smaller, more focused PRs so the community can review the changes more effectively.

More than 156 files are tests and snapshots.

[alissn]

I don't want this to happen again.

Reference: https://x.com/taylorotwell/status/1552452998546300929

[solomon-ochepa]

I don't want this to happen again.

Reference: https://x.com/taylorotwell/status/1552452998546300929

Do you realize at all that this same PR is to correct a problem you yourself introduced to this package?

Don't be too sentimental. Go through the PR and recommend changes if any.

Have you tested the PR and it's not working or do you find any bugs you'll like the author to fix?

Do you know the level of damages your changes cost us?

Let's join hands together and create solutions, not fighting each other all the time.

No one is paying us to contribute code here! Let's not feel so entitled or sentimental or judgemental please 🙏

[dcblogdev]

we're not looking to argue, its a massive PR, going to find some time to look over this and try out with a few apps I have.

The tests are all passing which is great 👍 with there being so many changes just want to check there's no unexpected issues that the tests don't check for.

[solomon-ochepa]

After merging this PR, we’ll likely be saying hi to a lot of new issues. 🤷‍♂️

Having 230 changed files is not normal. It would be better if the author could split this into smaller, more focused PRs so the community can review the changes more effectively.

Hi, I figured out a way to extract fractions of the PR into a separate micro PR. I've created a draft PR and will mark it ready once this is reviewed and merged.

While waiting for @dcblogdev and the rest of the QA team to complete the review, I'll review the codebase again to see if there is still room for more micro PRs.

Thanks for your observation and recommendation.

[solomon-ochepa]

I've squashed and optimized the commits to improve clarity. I recommend reviewing the pull request via the 14 commits rather than the 206 files changed—it’ll be much easier to follow the changes that way.

Thanks. @dcblogdev @alissn

[DeRaja]

Hello!

Thanks solomon-ochepa for showing me this solution. Can you check, if my solution is OK or easier?

I have found a totally different solution compared to what is proposed above. My solution is tiny and simple, easy to implement. Check it here:

https://github.com/nWidart/laravel-modules/issues/2073

[solomon-ochepa]

Hello!

Thanks solomon-ochepa for showing me this solution. Can you check, if my solution is OK or easier?

I have found a totally different solution compared to what is proposed above. My solution is tiny and simple, easy to implement. Check it here:

#2073

Your solution is a temporary fix, yes. But this PR is a better solution to solve the entire problem once and for all.

[dcblogdev]

@solomon-ochepa as soon as I pull in this PR I'm getting this error

   BadMethodCallException 

  Method Nwidart\Modules\Laravel\Module::app_path does not exist.

  at vendor/laravel/framework/src/Illuminate/Macroable/Traits/Macroable.php:115
    111▕      */
    112▕     public function __call($method, $parameters)
    113▕     {
    114▕         if (! static::hasMacro($method)) {
  ➜ 115▕             throw new BadMethodCallException(sprintf(
    116▕                 'Method %s::%s does not exist.', static::class, $method
    117▕             ));
    118▕         }
    119▕ 

  1   /Users/dave/Herd/packages/laravel-modules/src/Module.php:171
      Nwidart\Modules\Module::__call("app_path", [])
      +14 vendor frames 

  16  [internal]:0
      Illuminate\Foundation\Application::{closure:Illuminate\Foundation\Application::boot():1130}(Object(Mhmiton\LaravelModulesLivewire\LaravelModulesLivewireServiceProvider), "Mhmiton\LaravelModulesLivewire\LaravelModulesLivewireServiceProvider")

This happens when I have mhmiton/laravel-modules-livewire installed

[solomon-ochepa]

@solomon-ochepa as soon as I pull in this PR I'm getting this error

   BadMethodCallException 

  Method Nwidart\Modules\Laravel\Module::app_path does not exist.

  at vendor/laravel/framework/src/Illuminate/Macroable/Traits/Macroable.php:115
    111▕      */
    112▕     public function __call($method, $parameters)
    113▕     {
    114▕         if (! static::hasMacro($method)) {
  ➜ 115▕             throw new BadMethodCallException(sprintf(
    116▕                 'Method %s::%s does not exist.', static::class, $method
    117▕             ));
    118▕         }
    119▕ 

  1   /Users/dave/Herd/packages/laravel-modules/src/Module.php:171
      Nwidart\Modules\Module::__call("app_path", [])
      +14 vendor frames 

  16  [internal]:0
      Illuminate\Foundation\Application::{closure:Illuminate\Foundation\Application::boot():1130}(Object(Mhmiton\LaravelModulesLivewire\LaravelModulesLivewireServiceProvider), "Mhmiton\LaravelModulesLivewire\LaravelModulesLivewireServiceProvider")

This happens when I have mhmiton/laravel-modules-livewire installed

Thanks for the feedback. This probably happens because of the few changes I separated into new PRs.

I'm debugging it right away!

Thanks for the review and feedback, @dcblogdev

[solomon-ochepa]

Hi, @dcblogdev.

The issue has been fixed successfully - d95887e276d57e2dc54b5e8814de2bc8153633a8

Nevertheless, I need to work on the laravel-modules-livewire package ASAP

[dcblogdev]

thanks that error is fixed now, my tests are failing because Livewire can't see my components.

If you need a project that uses Livewire you can use https://github.com/laravel-modules-com/fuse

[solomon-ochepa]

thanks that error is fixed now, my tests are failing because Livewire can't see my components.

If you need a project that uses Livewire you can use https://github.com/laravel-modules-com/fuse

Can you share a screenshot or at least the error log, please?

Secondly, this PR is not about Livewire and has nothing to do with Livewire.

Livewire integration is handled completely by another package (laravel-modules-livewire).

I'm already working on a patch to fix every possible issue that may result from this PR.

I need some time to finish it. I'm currently in a meeting at my workplace.

Thank you, @dcblogdev

[dcblogdev]

I understand this is not about Livewire but my components work perfectly until I use this branch. tested on 3 different apps.

Livewire\Exceptions\ComponentNotFoundException
Unable to find component: [admin::admin.notifications-menu]

[solomon-ochepa]

I understand this is not about Livewire but my components work perfectly until I use this branch. tested on 3 different apps.

Livewire\Exceptions\ComponentNotFoundException
Unable to find component: [admin::admin.notifications-menu]

I understand why, and I'm not doubting you.

Have you forgotten? When the changes were made by @alissn he also changed a few codes in the Laravel-modules-livewire repo, which I'll fix ASAP.

[solomon-ochepa]

Hi, @dcblogdev , @nWidart and @DeRaja . I've submitted a complimentary PR for the Laravel modules livewire package.

Thank you.

[piotrczech]

@solomon-ochepa I totally see why you'd want to keep related changes together, especially if they were discovered while refactoring. But I still think splitting this into multiple, smaller PRs would really help the review process and make sure nothing critical slips through unnoticed.

As @alissn mentioned, having 210+ changed files is very hard to properly review, and merging such a big PR often leads to new issues afterwards. Smaller, more focused PRs would make it easier for everyone to understand the intention behind each change (like the stub adjustments or renaming transformers to resource).

This isn't about the changes being wrong, it’s just about keeping the PR easier to track, test, and discuss.

[solomon-ochepa]

@solomon-ochepa I totally see why you'd want to keep related changes together, especially if they were discovered while refactoring. But I still think splitting this into multiple, smaller PRs would really help the review process and make sure nothing critical slips through unnoticed.

As @alissn mentioned, having 210+ changed files is very hard to properly review, and merging such a big PR often leads to new issues afterwards. Smaller, more focused PRs would make it easier for everyone to understand the intention behind each change (like the stub adjustments or renaming transformers to resource).

This isn't about the changes being wrong, it’s just about keeping the PR easier to track, test, and discuss.

Yes, I understand that reviewing such large changes may lead to oversights.

However, we should also understand that splitting a highly coupled change could lead to inefficiency and untestable PRs.

Nevertheless, I'll check if there's anything not directly related to the main issue that can be removed.

Thank you.

[piotrczech]

However, we should also understand that splitting a highly coupled change could lead to inefficiency and untestable PRs.

I totally understand your point about wanting to avoid untestable or inefficient PRs. But the thing is, some of the changes here (like renaming Transformers to Resources and changing the stubs) don’t really seem tightly coupled to the main problem this PR is solving.

Nevertheless, I'll check if there's anything not directly related to the main issue that can be removed.

Actually, it even looks like you also recognized they could be separate, since you created another PR (#2091) with similar stub changes earlier, so they could clearly live in their own scope. Including them again here just because the other PR wasn’t merged feels a bit like pushing more changes into this one big PR.

I really think smaller, focused PRs are easier to review, test and understand, especially for the community. Right now, it really does feel less like we’re discussing the solution to the specific problem in the title, and more like trying to get as many unrelated changes merged as possible in one go.

[solomon-ochepa]

However, we should also understand that splitting a highly coupled change could lead to inefficiency and untestable PRs.

I totally understand your point about wanting to avoid untestable or inefficient PRs. But the thing is, some of the changes here (like renaming Transformers to Resources and changing the stubs) don’t really seem tightly coupled to the main problem this PR is solving.

Nevertheless, I'll check if there's anything not directly related to the main issue that can be removed.

Actually, it even looks like you also recognized they could be separate, since you created another PR (#2091) with similar stub changes earlier, so they could clearly live in their own scope. Including them again here just because the other PR wasn’t merged feels a bit like pushing more changes into this one big PR.

I really think smaller, focused PRs are easier to review, test and understand, especially for the community. Right now, it really does feel less like we’re discussing the solution to the specific problem in the title, and more like trying to get as many unrelated changes merged as possible in one go.

I get your point, and I totally agree with you.

I did more regarding the stubs, and many more other features such as the copy module command, move module, etc.

I'll move theses changes to the other PR or better stil create a new PR. But one thing for sure is that they will not reduce the count as other things are also modified in these files.

Give me a minute to remove these two off topic changes. Please, as you go continue reviewing the PR, you can always recommend any changes that can be move to a new PR.

Thanks for your time.