-
-
Notifications
You must be signed in to change notification settings - Fork 226
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
Add parameter types #455
Add parameter types #455
Conversation
This PR does too many things. Let's start small please. Let's start with parameter types only. Bumping to 8.1 does not yet feel necessary to me. We should not have many union types in parameters and |
Indeed, I went too far. Reworking. |
It did not change the type for Nothing needs to be updated in tests. |
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.
Looks good so far. Once this PR is merged, we might open a 2.0 branch where we bump to 8.1 and apply return types, WDYT?
* @return void | ||
*/ | ||
public function setLogger($logger) | ||
public function setLogger(callable $logger) |
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 is the logger a callable
anyway? 🙈
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.
Line 94.
$logger = $this->logger;
$logger($message);
You think we should change it to accept a Psr\Log\LoggerInterface
?
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.
We should look into that, yes. But probably in a separate PR.
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'm not going to do that, because it's used for console output directly.
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.
it's used for console output directly.
We could try to leverage Symfony\Component\Console\Logger\ConsoleLogger
. My problem is that a closure is a bad logging abstraction. While it works for the way the bundle uses it, it sucks if you want the logs to go anywhere else, e.g. you want to use a logging framework instead.
Deprecation: #462
Co-authored-by: Alexander M. Turek <[email protected]>
Sounds like a good idea. |
Thanks @GromNaN ! |
In
DoctrineMongoDBBundle
, we can't add parameter types to all the classes because they are not defined in the base class or the interface. https://github.com/doctrine/DoctrineMongoDBBundle/pull/812/files#diff-1571e1997e046b65a3fdffae3826e46e2a5a89adcce01ff580557d7eedc536e1R71Adding parameter types to all the methods should not be an issues for classes that extends them. But it might for interfaces.
Also upgrading to PHP 8.1 is required for some
mixed
and union types.