Support specifying namespace to generate classes in #57578
Conversation
This both fixes an infinite loop caused when the application namespace is an empty string (as it is in a domain-driven codebase like Winter CMS as there is no "root" namespace for the application) while also adding support for the --in option originally proposed in laravel#48571 by @innocenzi
| $this->getDefinition()->addOption(new InputOption( | ||
| 'in', | ||
| null, | ||
| InputOption::VALUE_REQUIRED, | ||
| "Specify a namespace to generate the {$this->type} class in" | ||
| )); |
There was a problem hiding this comment.
You could use promptForMissingArgumentsUsing()/getOptions() here
There was a problem hiding this comment.
It only needs to prompt for the argument in the case where the root namespace provided by the Laravel application container is empty, all other cases it should default to that value instead without prompting.
There was a problem hiding this comment.
Yeah, that's what getOptions() is for
There was a problem hiding this comment.
I'm not sure I understand what you're proposing exactly, can you do it as a code suggestion?
There was a problem hiding this comment.
Sure: https://github.com/laravel/framework/pull/57578/files#r2478427777
This looks like a lot but it really is just the first and last few lines. And GitHub seems to struggle with rendering the "before"/"old" part of the diff correctly
|
Tests are failing because of a byproduct of the ochestra testbench setup, the scaffolded files are being generated inside of vendor/orchestra/testbench-core/laravel/$absolutePathToOrchestraTestBenchCoreLaravel; investigating further |
… Testbench
Problem Context:
-----------------
The generator tests were failing because Orchestra Testbench creates a fake
Laravel application in vendor/orchestra/testbench-core/laravel/ with its own
composer.json that defines "App\\": "app/". However, the tests load Composer's
autoloader from the framework's vendor/composer/autoload, which has no
knowledge of the testbench app's namespace mappings.
The new resolvePathForClass() method (introduced in the composer-based path
resolution PR) relies on Composer's ClassLoader having registered namespace
mappings to determine where to place generated files. When it can't resolve
a namespace, it falls back to:
$this->laravel['path.base'] . '/' . str_replace('\\', '/', $name) . '.php'
This creates paths like:
/vendor/.../laravel/App/View/Components/Foo.php
^^^
Instead of:
/vendor/.../laravel/app/View/Components/Foo.php
^^^
The Problem with Test Generation:
----------------------------------
When generators use the CreatesMatchingTest trait with the --test flag, the
handleTestCreation() method attempts to extract the relative path using:
(new Stringable($path))->after($this->laravel['path'])
Where $this->laravel['path'] is "/vendor/.../laravel/app" (lowercase).
This fails to find "/app" within "/App/View/Components/Foo.php" (uppercase),
so the entire absolute path remains in the test name, creating deeply nested
incorrect paths like:
/vendor/.../laravel/tests/Feature/vendor/.../laravel/App/View/Components/FooTest.php
The Solution:
-------------
Register the testbench app's App\\ namespace with Composer's ClassLoader in
the test setUp() method. This allows resolvePathForClass() to succeed instead
of falling back, generating correct paths that match the filesystem case.
Alternative Approaches to Consider:
-----------------------------------
1. Adjust the fallback logic in getPath() to strip the root namespace and use
$this->laravel['path'] (preserving pre-PR behavior), but this could break
Winter CMS use cases where custom namespaces via --in don't match App\\.
2. Make handleTestCreation() more robust by using case-insensitive matching
or by extracting paths differently, though this doesn't address the root
cause of the fallback being used unnecessarily.
3. Have Orchestra Testbench itself register the App\\ namespace, though this
would require changes in testbench-core and wouldn't help users on older
versions.
The current approach fixes the root cause in the test environment without
affecting production behavior or breaking Winter CMS custom namespace support.
|
The failing test is passing locally, not sure why and can't rerun it on here. |
|
Not sure I want the namespace selection logic. 🤔 Can we fix the bug without that? |
3 participants
This both fixes an infinite loop caused when the application namespace is an empty string (as it is in a domain-driven codebase like Winter CMS as there is no "root" namespace for the application) while also adding support for the --in option originally proposed in #48571 by @innocenzi.
This PR takes it a step further to resolve the correct path for real namespaces so that namespaces outside of root namespace (which may or may not actually exist) can also be written to.