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:

• app/ path works with both default and custom configurations.
• Namespace is correctly converted to StudlyCase.
• No feature breakages in this branch.
• 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.

[piotrczech]

Hey! I was going through this PR and I had a few questions.

The PR seems a bit large (like said before) and I noticed there are some changes that feel slightly unrelated to the main scope - for example, changing the stub here, and also renaming things from Transformers to Resources.

Could you share the reasoning behind bundling these changes together in the same PR?

[piotrczech]

Stub changes

[solomon-ochepa]

Thanks for the review.

I contributed the initial format some time ago and while working on these changes I discovered they could be optimized.

I added them here so they can be reviewed together since they are just basic modifications.

Please let me know if they are done wrong, and I'll update them.

Thank you very much for your time.

[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.