Add void return to @ingroup entity_crud hook implementations

Created on 21 November 2024, 2 months ago

Problem/Motivation

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

Almost all entity_crud hooks return void, except hook_entity_preload

Steps to reproduce

Proposed resolution

Script coming soon, putting up MR for now

Remaining tasks

User interface changes

Introduced terminology

API changes

Fixed workspaces hooks to not return from void methods

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

entity 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 Failed
    2 months ago
    Total: 175s
    #345386
  • πŸ‡¦πŸ‡ΊAustralia mstrelan
  • Pipeline finished with Success
    2 months ago
    Total: 1014s
    #345395
  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    Missed a few that don't have the right docs, i.e. "Implements hook_ENTITY_TYPE_":

    • Drupal\block_content_test\Hook\BlockContentTestHooks::blockContentInsert()
    • Drupal\block_content_test\Hook\BlockContentTestHooks::blockContentPresave()
    • Drupal\block_content_test\Hook\BlockContentTestHooks::blockContentUpdate()
    • Drupal\block_content_test\Hook\BlockContentTestHooks::blockContentView()
    • Drupal\config_test\Hook\ConfigTestHooksHooks::configTestDelete()
    • Drupal\config_test\Hook\ConfigTestHooksHooks::configTestInsert()
    • Drupal\config_test\Hook\ConfigTestHooksHooks::configTestLoad()
    • Drupal\config_test\Hook\ConfigTestHooksHooks::configTestPredelete()
    • Drupal\config_test\Hook\ConfigTestHooksHooks::configTestPresave()
    • Drupal\config_test\Hook\ConfigTestHooksHooks::configTestUpdate()
  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    I think hook_entity_bundle_create is meant to be in the entity_crud group too, but the doc has @see instead of @ingroup, which we should fix too. That would give us the following extras:

    • Drupal\field_ui\Hook\FieldUiHooks::entityBundleCreate()
    • Drupal\jsonapi\Hook\JsonapiHooks::entityBundleCreate()
    • Drupal\entity_schema_test\Hook\EntitySchemaTestHooks::entityBundleCreate()

    And some more for the list in #4:

    • \Drupal\field_ui\Hook\FieldUiHooks::entityViewModePresave
    • \Drupal\field_ui\Hook\FieldUiHooks::entityFormModePresave
    • \Drupal\field_ui\Hook\FieldUiHooks::entityViewModeDelete
    • \Drupal\field_ui\Hook\FieldUiHooks::entityFormModeDelete
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    I'll finish reviewing later this is a lot.

    In the meantime can you add any with incorrect implements comments here https://www.drupal.org/project/drupal/issues/3488948 πŸ“Œ Confirm conversions where implements was incorrect Active

    Across all of these hook issues? We need to confirm the hook still has the extra type.

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

    Ok I took a look through and I don't see any other exceptions either.

  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    A few more:

    \Drupal\image_module_test\Hook\ImageModuleTestHooks::imageStylePresave
    \Drupal\layout_builder\Hook\LayoutBuilderHooks::fieldConfigDelete
    \Drupal\layout_builder\Hook\LayoutBuilderHooks::fieldConfigInsert
    \Drupal\menu_link_content\Hook\MenuLinkContentHooks::menuDelete
    
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    \Drupal\layout_builder\Hook\LayoutBuilderHooks::fieldConfigDelete
    \Drupal\layout_builder\Hook\LayoutBuilderHooks::fieldConfigInsert

    Can be combined:
    Delete one Move the hook to the other along with the implements line to the other. Rename the method as well so it's not referencing one hook or the other!

    I also confirmed these ones, I'll move them to πŸ“Œ Confirm conversions where implements was incorrect Active for completeness.

  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    Re #9: there are plenty of hooks that could be refactored one way or another, but that's not really in scope here, so I'd prefer to leave that out.

  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    Rebased and updated those in #4, #5 and #8.

  • Pipeline finished with Success
    16 days ago
    Total: 1201s
    #386969
  • Pipeline finished with Success
    15 days ago
    Total: 452s
    #388120
  • Pipeline finished with Success
    14 days ago
    Total: 616s
    #389145
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Needs a rebase

    If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

  • Pipeline finished with Success
    1 day ago
    Total: 682s
    #401334
  • Status changed to Needs review about 24 hours ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Rebase seems valid.

Production build 0.71.5 2024