Add string return type to all hook_help implementations

sourav_paul → changed the visibility of the branch 3483146-add-string-return to hidden.

Unfortunately many of these don't return anything if the switch statement doesn't find a matching case. We probably need to update those to return an empty string or perhaps null or false. The empty string would be the simplest, null or false would require a union type signature.

We also need to regenerate the phpstan baseline.

Facing issue on regenerating phpstan-baseline, I will appreciate if anyone could help by regenerating the baseline...

I can't right now but here is how you can:

Replace the current baseline file with:

<?php declare(strict_types = 1);

$ignoreErrors = [];


return ['parameters' => ['ignoreErrors' => $ignoreErrors]];

Then run this:

./vendor/bin/phpstan analyze --configuration=./core/phpstan.neon.dist --memory-limit=-1 --generate-baseline=core/.phpstan-baseline.php

#8 You shouldn't need to replace the current baseline, the command will overwrite what you have already.

I'm not quite convinced about return an empty string though. I guess it's fine, but I think returning NULL or FALSE would be better, as per jsonapi_help, but we'd then also need to update the api doc to allow that in the @param annotation.

While we're at it, why does every hook_help implementation have a switch statement, just to check a single case? Is there a scenario where hook_help would return something for multiple routes?

You shouldn't need to replace the current baseline, the command will overwrite what you have already.

Yes, but the memory requirements are far higher, I know the only way to get the process to run on my laptop reliably is to do the above.

Gotta rebase first...

Sorry for the churn, phpcs was passing locally but failing here for some reason.

There are a couple of failures that look relevant, but phpstan is passing so someone should be able to take another look.

I fixed a couple, some hook_help are not returning strings.

Either they have an if within the switch that doesn't return anything (which I fixed on system and language by adding a default and returning output at the end)

Or they are returning render arrays:

return ['#markup' => 'Help text from help_page_test_help module.'];

I wrapped those in

tags instead, but I'm not sure that is what we want to do.

Also a bunch returned NULL or t() directly.

Yeah this is still getting some relevant failures, I can't dig in further right now.

I think we should postpone this until we get the conversions done, there are some failures that need deeper review and this won't easily rebase.

Ok the conversion issue landed, unfortunately that means this needs to be redone.

There were several issues I identified that still need addressing too.

For anyone wanting to pick this up, here is a command to add the return type. Will need to redo all the other fixes for functions that don't return anything though.

find . -type f -path "*/src/Hook/*.php" -exec sed -i "/#\[Hook('help')\]/ {N;s/\(public function \w*([^)]*)\)/\1: void/}" {} +

I'm working on this.

Here is the command updated to string instead of void:

find . -type f -path "*/src/Hook/*.php" -exec sed -i "/#\[Hook('help')\]/ {N;s/\(public function \w*([^)]*)\)/\1: string/}" {} +

I'm going to expand the scope a bit since I already have to review each one I'm going to also move them to their own class so they don't get loaded on each hook call.

nicxvan → changed the visibility of the branch 3483146-hook-help to hidden.

nicxvan → changed the visibility of the branch 3483146-add-string-returntype to hidden.

nicxvan → changed the visibility of the branch 11.x to hidden.

I intentionally left out workspace since it's being addressed elsewhere in more detail.

There is also HelpTestHooks which is set to void since it's testing a missing return.

I can remove baseline rebase and regenerate in the morning and fix the cs failure.

Appear to be 2 valid open threads.

Answered both

To answer the question about what the caller does, the HelpController does an 'empty' check.

Clearly, adding string to all these hooks is not sufficient to cover the current values returned, which are string, array and NULL. Is there a reason that all these hooks can't use array|null|string? Then the default return value can be NULL instead of an empty string, '', which seems awkward.

Yes we also need Markup i think

I think the hook definition should return string|\Stringable|array|NULL but the implementations should return the smallest subset of these that is applicable.

Refactored help() functions so they have output defined and use only one return statement.

@mstrelan mention in the meta the gain may be minimal but still think worth to it. Believe all threads have been resolved and merge conflicts addressed. Going to go on a limb and say this is good.

Yes, #35 make sense. Since this affects the UI has anyone manually tested to ensure there are no mistakes?

I started to review the MR and stopped because of the out of scope changes, such as refactoring done to separate an if block into case statements and the broad application of having one return statement. The number of lines changes would be much less and the review would be faster if this limited to the changes necessary. I need a second opinion on the scope here.

Yeah if we're going to stick to 35 this probably needs to start over.
I wonder if we split this one into a few chunks since it's been thorough the wringer and needs manual updates for each implementation to choose the right return types.

Yea, I looked at the MR again, and agree with myself earlier that there are too many out of scope changes. The changes do, on a first look, appear to be good refactoring. That just needs to be in a separate issue.

I like to think that one issue for the return type will suffice. Without the extra changes it will be significantly easier to read.

It's not possible to do return only because some implementations do not return anything. They need to at least be refactored to return NULL. Fwiw I initially recommended adding a return to the end of these functions but the feedback was that it was confusing and there was a preference to initialise the return value at the start and return at the end.

So, should we split this to just fix the ones that always return something and address the others separately? Or just leave it baselined and wait till hook_help is replaced entirely by help topics?

This now needs a rebase.

Are we willing to accept the technically out of scope changes? Or should this become a meta?

The changes need to happen for the return types so I'd be inclined to keep this issue, but I don't want to rebase if it needs more discussion.

I spoke with @quietone about this and have an idea for a simpler MR, hoping to post something in the next few hours.

Great! I'll keep an eye out!

Added MR !11237 which is a much smaller diff, at +150 -527 instead of +530 -869. Most of the removals are from the baseline.

It is split up in to 4 commits. I think if we wanted to split this issue we could just include the ?string returns and leave the "other" returns for a follow up.

nicxvan → changed the visibility of the branch 3483146-hook-help2 to hidden.

This is so much better!

5 modules were missed:

• File module
• layout discovery
• packagemanager
• Experimentalmoduletestrequirements
• Workspaces

#50

File, Package Manager and Workspaces already has a return type on their hook_help implementations. Have updated Layout Discovery and Experimental Module Test Requirements.

Looks great now and I think that's all of the hooks!

@mstrelan, thank you!

This doesn't initialize the output string anymore and adjust the code for that, which was nice refactoring. Now, this just adds a NULL return and, of course, add the return type. Very simple to review.

And, in light of 🌱 Deprecate hook_help() and combine with Topics Active , which @mstrelan reminded me about, it makes sense to make minimal changes here.

Committed 367e25f and pushed to 11.x. Thanks!