[META] Add return types to hook implementations

Created on 24 October 2024, about 2 months ago

Problem/Motivation

In 🌱 [Meta] Implement strict typing in existing code Active we want to add strict typing to existing code.

In ✨ Enforce return types in all new methods and functions RTBC 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 πŸ“Œ [PP-2] Convert Core hook implementations to Class or method based implementations Active 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

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

base system

Created by

πŸ‡¦πŸ‡ΊAustralia mstrelan

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @mstrelan
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    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.

  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    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 πŸ“Œ Add void return type to all *_alter hook implementations Active this drops to 587.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

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

  • πŸ‡©πŸ‡ͺGermany tstoeckler Essen, Germany

    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: πŸ“Œ Add return types to update_test_* hooks Active

    Hope that's OK and would love a review.

  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    @tstoeckler that's great. I realised we could probably tackle all hook_update_N implementations in one hit. Opened πŸ“Œ Add return types to hook_update_N implementations Active if anyone wants to work on it.

  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I think that's most of them!

Production build 0.71.5 2024