-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DX] [DI] Throw more helpful error when shortcutting global classes #22153
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -48,6 +48,14 @@ public function process(ContainerBuilder $container) | |||
if ($definition->getFactory()) { | |||
throw new RuntimeException(sprintf('Please add the class to service "%s" even if it is constructed by a factory since we might need to add method calls based on compile-time checks.', $id)); | |||
} | |||
if (class_exists($id) || interface_exists($id)) { |
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.
false
should be added as second argument to interface_exists
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.
Why that? We want to check whether the interface can be loaded by the project's autoloader, not whether it has been loaded during compilatiom.
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.
Because the class_exists will already have called the autoloader
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.
Valid point (had a few beers tonight ;) )
throw new RuntimeException(sprintf( | ||
'Service "%s" appears to reference a class in the global namespace, and does ' | ||
.'not have a class defined. Leaving out the class property is only allowed ' | ||
.'for namespaced classes. Specify the class explicitly to get rid of this error.', |
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.
Full message could be eg.:
The definition for "%s" has no class. Leaving out the "class" attribute is only allowed for namespaced classes. Please specify this attribute explicitly to get rid of this error.
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.
That is imho less clear as the "appears to reference a class in the global namespace" part is key to the DX - it explains that what the developer is trying to do is not allowed, and then why. In your suggestion it's not emphasized that the global namespace is the issue.
I do use the word property instead of attribute. Is that convention?
…asses As discussed in symfony#22146 the error message received when trying to use a class in the global namespace as a service without defined class is confusing. Helpful information was added pointing out this current limitation.
Improved exception clarity and added parameter to |
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 @curry684. |
…bal classes (curry684) This PR was merged into the 3.3-dev branch. Discussion ---------- [DX] [DI] Throw more helpful error when shortcutting global classes | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? |no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22146 | License | MIT As discussed in #22146 the error message received when trying to use a class in the global namespace as a service without defined class is confusing. Helpful information was added pointing out this current limitation. Commits ------- b9e7b4f [DependencyInjection] Throw helpful error when shortcutting global classes
As discussed in #22146 the error message received when trying to use a class in the global
namespace as a service without defined class is confusing. Helpful information was added
pointing out this current limitation.