Missing NULL in hook_file_download() return documentation

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

I wonder if we can explicitly document -1 in return type. PHPStan allows specifying scalar values as types in PHPDocs.

https://phpstan.org/writing-php-code/phpdoc-types#literals-and-constants

I don't understand why review bot fails on phpstan. MR check looks good.
Add tag to not ignore bot.

@chi this looks like a good suggestion. I believe there are more places where this can be done.

@niklan In the issue summary

The documentation for hook_file_download() contradicts itself because NULL is a valid return type that is not listed in @return.

I think you mean the documentation is 'contradictory'. I dont understand the rest of the sentence. How is the documentation contradictory? I can see it has something to do with a NULL return type. But you dont explain what. Can you please edit it.

Re #9.
The documentation for the @return tag consists of two parts: the type hint and a free-form description. I believe the issue is that they contradict each other.

I'm checking
#ContributionWeekend2025 #WarsawContributionWeekend2025

once more with tags

mentoring on this

Tested, looks good for me

@chi RE: #12 Yes, I groped through the darkness as far as i could but #29 shines a light on the issue. The issue summary looks good to go. The MR code is hard to disagree with, too. Move to RTBTC?

I mean the only problem i could envisage is if it turns out that any of the 3x return types is wrong. But that would seem outside the scope.

1. Where does the function actually return int?
2. It doesn't return anything if the if condition is not met (void).

I think we should add a :?array return type hint and actually return NULL if the if condition is not met.

Ok, actually see file_file_download(), it returns void in all the cases instead of NULL. I think that should be included in the doc comment.

I think #19 needs test coverage so would be in a follow-up.

It doesn't return anything if the if condition is not met (void).

Right. Needs to be fixed.

I think we should add a :?array|int

Union types and the nullable type notation cannot be mixed. Anyway, as this issue is about @return tag I propose we don't add the typehint here. This is managed in other issue. 📌 [META] Add return types to hook implementations Active .

Ok, actually see file_file_download(), it returns void in all the cases instead of NULL.

But we can't use void as the return type and combine it with other types. Does that mean all hook implementations must explicitly return NULL and void return should be deprecated somehow?

Re #20. Turns out all implementations in Drupal core use void instead of null. Given, can only be used as a standalone type we have to update them. Otherwise we will never be able to add native return type hint to this hook. I agree, that needs to be done in a follow-up.

graber → changed the visibility of the branch drupal-3501398-3501398-missing-null-in to hidden.

This will not break any contrib modules implementing this hook and still returning void, because we don't have a type hint and it is still a hook so even if we had a type hint in file.api.php file, hook implementations wouldn't necessarily add those and we don't check the return type after invoking this hook.
However, we need a change record so contrib module maintainers will now return NULL in their implementations for consistency.

Looks good, thanks!