Wrong return type in FileSystemInterface::tempnam

Well, there are a few other cases like this. I suppose the issue needs to be expanded to fix all of them.

ramprassad → changed the visibility of the branch 3499213-wrong-return-type to hidden.

The change being done here is not in agreement with the Drupal coding standards for @return → . The standard states

The @return tag is followed by an optional data type indicator, → and then a newline. The following paragraph is considered by the API module to be documentation.

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?

@quietone Why do you think this violates Drupal coding standards?

 * @return string|false
 *   The new temporary filename, or FALSE on failure.

I 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.

For the PHP built-in types, use the following names:

• ...
• false (NOT "FALSE", see bool)

I checked core code. Found 162 occurrences of false in PHPDoc tags. FALSE is not used at all.

I also concerned about how to find the instances that this is suggesting be changed. A simple grep will not work.

Instances found with grep needs to be filtered manually by comment referring to false.
Like this.

The new temporary filename, or FALSE on failure.

🐛 StreamWrapperInterface realpath() and dirname() return docblocs are is inconsistent with documented use, actual Core implementation, and intention Needs work 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.

I 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?

I think that because Boolean is the data type, not false or true. https://www.php.net/manual/en/language.types.php.

@quietone true and false are covered there in value types section.

and how will we prevent future problems?

For 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.

What is the plan here so that we know that they are all fixed

Some missed would be better than none fixed.

PHPStan 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

@chi, thanks

Changing 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.

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.

The 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.

Restoring status.

No need to credit me for this. All I did was hit the rebase link.