Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Config] More accurate message on invalid config builder#40994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Nyholm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you. Well done with the tests!
| $alias = Container::underscore(substr($namespace,15, -6)); | ||
| if (false !==strpos($alias,'\\')) { | ||
| thrownewInvalidArgumentException(sprintf('Nested bundle config class "%s" could not be handled.',$namespace)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Technically, we are not talking about "bundles" but "extensions".
Could we be more descriptive here?
Something more similar to this?
Could not find or generate class "%s". Make sure to use the "root" ConfigBuilder class. Ie use "Symfony\Config\FrameworkConfig" and not Symfony\Config\Framework\CacheConfig"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have tried to make message more descriptive, but without concrete classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh. I missed this comment.
I do agree that not usingSymfony\Config\Framework\CacheConfig may be better, but it is also very hard to understand what "nested classes" really mean. Im not sure that my suggestion is the best.
src/Symfony/Component/DependencyInjection/Tests/Loader/PhpFileLoaderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
1c3b4d7 to5ece904Comparesrc/Symfony/Component/DependencyInjection/Tests/Loader/PhpFileLoaderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/DependencyInjection/Loader/PhpFileLoader.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Throw exception when try to autowire nested bundle config instead of ConfigBuilder.Also renamed test class AcmeConfigBuilder to AcmeConfig according config builders auto naming.
5ece904 to0501eccCompareNyholm left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you
Im happy with this PR. I just had one small question left.
src/Symfony/Component/DependencyInjection/Tests/Fixtures/config/nested_bundle_config.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
derrabus commentedMay 2, 2021
Thank you@a-menshchikov. |
Throw exception when trying to autowire nested bundle config instead of ConfigBuilder (related Doc PRsymfony/symfony-docs#15300).
Also renamed test class AcmeConfigBuilder to AcmeConfig according config builders auto naming.
Inspired by a chat with@Nyholm