Add void return type to all hook_install and hook_uninstall implementations

Created on 24 October 2024, 28 days ago

Problem/Motivation

See πŸ“Œ [META] Add return types to hook implementations Active

Steps to reproduce

Find all occurrences in the baseline:

grep "Function .*_install\\\\.* has no return type specified" core/.phpstan-baseline.php
grep "Function .*_uninstall\\\\.* has no return type specified" core/.phpstan-baseline.php

Proposed resolution

Add : void to each of them.

These nifty commands will find and replace all occurrences that come shortly after "Implements hook_install()" and "Implements hook_uninstall()":

find . -type f -exec sed -i '/Implements hook_install()/,/\(function .*_install(.*)\) *{/!b;/\(function .*_install(.*)\) *{/s/\(function .*_install(.*)\) *{/\1: void {/' {} +

find . -type f -exec sed -i '/Implements hook_uninstall()/,/\(function .*_uninstall(.*)\) *{/!b;/\(function .*_uninstall(.*)\) *{/s/\(function .*_uninstall(.*)\) *{/\1: void {/' {} +

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

install system

Created by

πŸ‡¦πŸ‡ΊAustralia mstrelan

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

Merge Requests

Comments & Activities

  • Issue created by @mstrelan
  • Pipeline finished with Canceled
    28 days ago
    Total: 151s
    #318975
  • πŸ‡¦πŸ‡ΊAustralia mstrelan
  • Pipeline finished with Success
    28 days ago
    Total: 1205s
    #318977
  • πŸ‡³πŸ‡±Netherlands bbrala Netherlands

    Almost got scared at this one, went through all function, and the count did add up, nothing missed.

    One thing that has been missed though is perhaps some tests:

    Drupal/FunctionalTests/Install/ has multiple tests that actually write an install function to file. Shouldn't those also be changed to add the return?

    Example:

    file_put_contents("$path/my_distribution.install", "<?php function my_distribution_install() {\Drupal::entityTypeManager()->getStorage('path_alias')->create(['path' => '/user/1', 'alias' => '/root-user'])->save();}");
    
  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    Thanks for the review. I'm not sure there's much point updating these files generated in tests. The point of this issue is to reduce the baseline and maybe one day get to a point where there is a phpcs rule that enforces this, but the contents of this generated file doesn't really conform to any other coding standards. It will never be scanned by phpcs nor phpstan. It also adds to the cognitive load for reviewer or committer as it looks different to the rest of the diff.

    Setting back to Needs Review, if you feel strongly set it back again and I'll work on it.

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

    Would agree with #5. Since these are generated for tests believe should be fine. If we had any hooks generated from starterkit for live themes then I'd lean towards updating.

    Current changes seem fine to me though.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    I got the same results and had the same questions as @bbrala which is answered in #6. Answers that I agree with.

    Assigning to myself to commit in a day or two.

    • quietone β†’ committed 42c86202 on 11.x
      Issue #3483057 by mstrelan, bbrala, smustgrave: Add void return type to...
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Committed 42c8620 and pushed to 11.x.

    Thanks!

Production build 0.71.5 2024