Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
There a number of return phpdocs that claim to return bool, but can only return false. This is usually in a union type, for example in \Drupal\Core\File\FileSystemInterface::tempnam:
* @return string|bool
* The new temporary filename, or FALSE on failure.
This subtle difference is important when you narrowing types for stat analysis.
Proposed resolution
Change bool to false, e.g. @return string|false
Limit the scope to @return phpdocs only, not @param, not @var and not native return type declarations.
| Comment | File | Size | Author |
|---|
Issue fork drupal-3499213
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
chi commentedWell, there are a few other cases like this. I suppose the issue needs to be expanded to fix all of them.
Comment #5
ramprassad commentedComment #8
chi commentedComment #9
quietone commentedThe change being done here is not in agreement with the Drupal coding standards for @return. The standard states
So, based on that this is a won't fix. Or, needs an issue in the Coding Standards project to discuss.
I also concerned about how to find the instances that this is suggesting be changed. A simple grep will not work. Ideally, all the coding standards can be enforced with a sniff, so that regressions are not introduced. Is there a sniff for this?
Comment #10
chi commented@quietone Why do you think this violates Drupal coding standards?
Comment #11
mstrelan commentedI don't see this as a coding standard issue. I also don't see how this violates existing coding standards. The only confusion I can see is that we usually write
FALSE, but that's when we're referring to it as a value, not as a data type. The data type indicator notes in the coding standards clarify this.Comment #12
chi commentedI checked core code. Found 162 occurrences of
falsein PHPDoc tags.FALSEis not used at all.Comment #13
chi commentedInstances found with grep need to be filtered manually by comment referring to false.
Like this.
Comment #14
cmlara#3271894: StreamWrapperInterface realpath() and dirname() return docblocs are is inconsistent with documented use, actual Core implementation, and intention as related, it discusses some of the streamWrapper returns including LocalStream::getLocalPath()
Not reviewed the MR, however a general +1 to the concept to cleanup documented return types as this hurts contrib that run at PHPStan and will eventually mask errors in Core that PHPStan can detect.
Agree this isn't a Coding Standard issue.
Comment #15
quietone commentedI think that because Boolean is the data type, not false or true. https://www.php.net/manual/en/language.types.php. And I do understand that this change helps.
There are currently 1064 instances of bool in and @return statement. What is the plan here so that we know that they are all fixed and how will we prevent future problems?
Comment #16
chi commented@quietone true and false are covered there in value types section.
Comment #17
cmlaraFor now that may just have to be the Human Review process.
Core could create a PHPCS rule that the description must contain a format like “True if . False if .” when BOOL is used however that might be a bit more disruptive. I would suggest such should be a follow-up issue as fixing the broken docs is the bigger concern.
Core adopting higher level PHPStans would also help long term but is outside the scope or this issue.
Some missed would be better than none fixed.
Comment #18
chi commentedPHPStan currently does not have a rule for checking less specific return types. Psalm does.
I'v just created a feature request for PHPStan project.
https://github.com/phpstan/phpstan/issues/12442
Comment #19
quietone commented@chi, thanks
Comment #20
mstrelan commentedChanging to NW because we have an MR. Updated the IS to expand the scope to all @return docs. It might be possible to detect and automate this, but in the meantime we might as well fix the ones we've already identified. We could open a follow up to automate it.
Comment #21
mstrelan commentedComment #22
borisson_I think this looks great. I agree with @quietone that i'd prefer to see this go in when we already have a rule.
However since there's not a phpstan rule for this yet and it seems like it's not too many changes for me this seems like it is good to go.
It's an improvement of the current status.
I think the "controversial" point is not controversial at all and I've also verified it can only return false.
Comment #23
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #25
dcam commentedRestoring status.
No need to credit me for this. All I did was hit the rebase link.
Comment #26
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #27
dcam commentedRestoring status.
Again, no need to credit me for this. All I did was hit the rebase link. I don't know why it thinks the patch doesn't apply, but it rebases cleanly.
Comment #28
alexpottCommitted 05d72b3 and pushed to 11.x. Thanks!