Problem/Motivation

In #3050720: [Meta] Implement strict typing in existing code we want to add strict typing to existing code.

In #3461318: Enforce return types in all new methods and functions we added thousands of methods and functions without a return type to the phpstan baseline.

Adding return type declarations to methods in classes can have backwards compatibility implications since these can be extended in contrib and custom modules. But fixing hook implementations is easy, because they can not be extended, at least while they are still procedural. The return type of each hook should also be predefined by the documentation of the hook.

This meta is to fix as many of these as possible.

Note that there is no inherent value in hook implementations having return types, but it reduces the size of the phpstan baseline, and gets us closer to a place where we can have return types everywhere.

Steps to reproduce

Find the number of functions with no return type in the phpstan baseline. Note not all of them are hooks, but many of them are.

$ grep "Function .* has no return type specified" core/.phpstan-baseline.php | wc -l
1907

Grepping this further reveals the following most common hooks occurrences:

hook_preprocess_HOOK: 263
hook_help: 83
hook_form_alter: 73
hook_theme: 45
hook_removed_post_updates: 41
hook_update_last_removed: 37
hook_install: 36
hook_uninstall: 11

That gives us 553 functions to update.

Proposed resolution

Open a child issue for each of the following:

  • Add void return type to all preprocess hook implementations
  • Add string return type to all hook_help implementations
  • Add void return type to all hook_form_alter implementations
  • Add array return type to all hook_theme implementations
  • Add array return type to all hook_removed_post_updates implementations
  • Add int return type to all hook_update_last_removed implementations
  • Add void return type to all hook_install and hook_uninstall implementations

If any outliers are identified in these issues they should be called out in separate issues to keep each of these issues simple.

Ideally these should be done before #3472214: [META] Convert Core hook implementations to Class or method based implementations to avoid having to update the baseline as these functions are moved.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Comments

mstrelan created an issue. See original summary.

nicxvan’s picture

nicxvan commented

Thank you for creating this. I was able to help get a few of these in before the conversion landed.

hook_help unfortunately had deeper issues I could not resolve before that got in.

The preprocess and schema were not converted as part of that issue so the work done on those issues is still valid, baseline just needs to be regenerated.

mstrelan’s picture

mstrelan commented

Some statistics so far.

We started with 1907 procedural functions with no return type specified. Hooks are OOP now, so they match a different grep pattern.

If we use the original grep pattern we can see how many non-hook functions are left in the baseline, which are out of scope for this meta:

$ grep "Function .* has no return type specified" core/.phpstan-baseline.php | wc -l
511

That means we had roughly 1396 hook implementations to update.

If we use this new grep pattern we can see how many hook implementations with no return type are remaining:

$ grep "Method Drupal\\\\.*\\\\Hook\\\\.* has no return type specified" core/.phpstan-baseline.php | wc -l
834

After #3487666: Add void return type to all *_alter hook implementations this drops to 587.

nicxvan’s picture

nicxvan commented

Each baseline item is 6 lines too, that means you've eliminated over 4800 lines from baseline with another 3000 to go!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Now that a bunch of the bigger "per-hook" issues landed, I looked into tackling this on a "per-module(ish)" basis and started with the biggest offender: #3490638: Add return types to update_test_* hooks

Hope that's OK and would love a review.

mstrelan’s picture

mstrelan commented

@tstoeckler that's great. I realised we could probably tackle all hook_update_N implementations in one hit. Opened #3490954: Add return types to hook_update_N implementations if anyone wants to work on it.

nicxvan’s picture

nicxvan commented

I think that's most of them!

chi’s picture

chi commented

Is this duplicate of #3229216: Add native return types to hook definitions?

nicxvan’s picture

nicxvan commented
Related issues: +#1158720: Improve text for parameter type hinting in function declaration

Yep! I closed it as a duplicate since this one has nearly completed the task.

Also added the related issue here.

mstrelan’s picture

mstrelan commented

I re-opened #3229216: Add native return types to hook definitions because this doesn't yet cover all of it. I've explained in that issue.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.