-
Notifications
You must be signed in to change notification settings - Fork 39
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
repository_class configuration option could be replaced with FQCN repository name #32
repository_class configuration option could be replaced with FQCN repository name #32
Conversation
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.
Great work done 👍
May I ask you to make aggregate_type
and aggregate_translator
required (here)?
$repositoryClass = $repositoryConfig['repository_class'] ?? $repositoryName; | ||
|
||
if (! class_exists($repositoryClass)) { | ||
throw new RuntimeException(\sprintf( |
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.
Please remove the leading \
for consistency
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.
👌
Docs are already available in #27 |
done, thanks for taking care of docs. |
@@ -176,7 +176,7 @@ private function loadEventStore(string $name, array $options, ContainerBuilder $ | |||
$repositoryClass = $repositoryConfig['repository_class'] ?? $repositoryName; | |||
|
|||
if (! class_exists($repositoryClass)) { | |||
throw new RuntimeException(\sprintf( | |||
throw new RuntimeException(sprintf( |
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 was the \
backslash removed from sprintf
? This gives a minimal performance gain.
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've read somewhere, that the performance difference will be gone in future php version and you shouldn't use this except when needed (but I can't find the sources for this again at the moment). Currently we don't do this within all the prooph components, so even if my statement above would be invalid, let's keep it consistent.
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.
and tools like cs-fixer can always change it in entire project
Thank you @kejwmen |
Implements #5.