-
Notifications
You must be signed in to change notification settings - Fork 8k
ext/reflection: make getDocComment() methods return empty string instead of false #18928
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
...ead of false Returning false is just annoying for non-existent doc comment
b08f4d1 to
d03aa48
Compare
Hmm, so I'm torn
On the one hand, indicating no comment with an empty string suggests that maybe there is a comment but it was just empty, while false makes it clear there was absolutely no comment
On the other hand, https://3v4l.org/fJBKT suggests that you cannot have an empty comment, the /** and */ are part of the comment that is returned, so an empty string can't really indicate an empty comment
But this would also break code that checks the return without just if ($ref->getDocComment()), e.g. if ($ref->getDocComment() !== false)
I only needed to go to the second page of the GitHub search results for "->getDocComment()" lang:php to find such code:
https://github.com/salient-labs/toolkit/blob/ee9034a67f1d90201c38f304638fac8ea8a198f6/src/Toolkit/PHPDoc/PHPDocUtil.php#L55
At the very least, this should probably have a discussion on the internals list
I also saw some existing code that works around the false return, using ?: or a (string) cast - is that not possible for Symfony?
Hmm, so I'm torn On the one hand, indicating no comment with an empty string suggests that maybe there is a comment but it was just empty, while false makes it clear there was absolutely no comment On the other hand, https://3v4l.org/fJBKT suggests that you cannot have an empty comment, the
/**and*/are part of the comment that is returned, so an empty string can't really indicate an empty comment But this would also break code that checks the return without justif ($ref->getDocComment()), e.g.if ($ref->getDocComment() !== false)I only needed to go to the second page of the GitHub search results for
"->getDocComment()" lang:phpto find such code: https://github.com/salient-labs/toolkit/blob/ee9034a67f1d90201c38f304638fac8ea8a198f6/src/Toolkit/PHPDoc/PHPDocUtil.php#L55At the very least, this should probably have a discussion on the internals list
I also saw some existing code that works around the false return, using
?:or a(string)cast - is that not possible for Symfony?
It is possible, but I'm not sure that there is a big point in it. But I will raise this on internals.
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.
Fine with me to restrict this type, but breaks subclassing, due to the return type changing
Fine with me to restrict this type, but breaks subclassing, due to the return type changing
The return type is mark as tentative, so you'd "just" get a warning. Which is also why I want this to land prior to PHP 9
Internals didn't think this was worthwhile: https://news-web.php.net/php.internals/127747
Returning false is just annoying for non-existent doc comment
Found this while seeing the impact of #18879 on Symfony's test suite.
And discussing with @nicolas-grekas it seems to just make more sense to fix PHP.