Add scalar typehints/return types #645
Conversation
|
Can we have a rebase here, please? Thanks a lot! |
|
Done. |
| } | ||
|
|
||
| public function getTitle() | ||
| public function getTitle(): ?string |
There was a problem hiding this comment.
Not for this PR, but we should make the title (and slug) mandatory so this return value can never be null, don't you think?
There was a problem hiding this comment.
I thought the same at beginning, but they are already mandatory in DB & FormValidation. so when I tested the real app (/es/admin/post/new):
Type error: Return value of App\Entity\Post::getTitle() must be of the type string, null returned
The cause is on form initialization, it needs mapDataToForms() and read each defined form field from underlying object (default values), so PropertyAccessor->readProperty(array(object(Post)), 'title') calls to Post->getTitle() and boom! null value.
The solution could be set a fake or default title/slug for new Post (before form initialization), now that Slugger::slugify() is static, it can be easier. WDYT? then we would change it to strict string.
There was a problem hiding this comment.
I'm not sure it's worth it. Now that you explained that, I remember that I faced this issue too a while ago. I needed to put ?string in an entity because of the Form behavior that you mentioned. So let's keep it. Thanks.
| * {@inheritdoc} | ||
| */ | ||
| public function transform($array) | ||
| public function transform($array): string |
There was a problem hiding this comment.
array typehint is missing. What if we rename the argument too from $array to $tags ?
There was a problem hiding this comment.
arraytypehint is missing.
Yep, but I can't do that, because declaration must be compatible with DataTransformerInterface->transform(value).
What if we rename the argument too from $array to $tags ?
👍 Done.
|
It's nice to have PHP 7.1 type hints and return types everywhere. Thanks a lot for contributing this feature! |
|
Glad to help 🎉!! |
2 participants
Fixes #639 (comment)