-
-
Notifications
You must be signed in to change notification settings - Fork 261
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
Improve typehint #382
Improve typehint #382
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.
Checking the configuration, the revision id seems to accept anything:
->scalarNode('revision_id_field_type')->defaultValue('integer')->end() |
Maybe we could restrict the config to "integer" and "string".
It would be a BC-break no ? I think we will be able to restrict only for the next major... |
Yes. We should deprecate the types that we consider invalid before. |
@phansys There is the following code:
Would it work with string ? |
I think the |
ae65aa0
to
06e89b2
Compare
06e89b2
to
9241563
Compare
->scalarNode('revision_id_field_type')->defaultValue('integer')->end() | ||
->scalarNode('revision_id_field_type') | ||
->defaultValue('integer') | ||
// NEXT_MAJOR: Use validate() instead. |
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.
what about enumNode instead?
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.
Didn't know this.
protected $revision; | ||
|
||
public function __construct($className, $id, $revision) | ||
public function __construct(?string $className, ?array $id, $revision) |
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.
public function __construct(?string $className, ?array $id, $revision) | |
/** | |
* @param int|string|null $revision | |
*/ | |
public function __construct(?string $className, ?array $id, $revision) |
Subject
I am targeting this branch, because BC.
The
$revision
param is often restrict toint
but could work with string too. (and SonataAdmin is usingint|string
).The
$id
seems to only work withint|string|array
if I read the code.I also add some typehint for constructor or Exception/Object we're using.
This will fix some errors from https://github.com/sonata-project/SonataDoctrineORMAdminBundle/pull/1386/checks?check_run_id=2265615169
Changelog