Coming from #1679594: node_requirements() breaks Drupal installer

Problem

The node module currently implements hook_requirements() in node.module.

Hook_requirements() documentation states:

Note that this hook, like all others dealing with installation and updates, must reside in a module_name.install file, or it will not properly abort the installation of the module if a critical requirement is missing.

Solution

Move node_requirements() from node.module to node.install.

CommentFileSizeAuthor
#6 drupal-move_hook_requirements_install-2106783-6.patch3.26 KBbneil
#1 drupal-move_hook_requirements_install-2106783-1.patch3.43 KBbneil

Comments

bneil’s picture

bneil commented
Assigned: bneil » Unassigned
Status: Active » Needs review
StatusFileSize
new drupal-move_hook_requirements_install-2106783-1.patch3.43 KB
bneil’s picture

bneil commented
Issue tags: +Needs backport to D7
kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me, yay for living up to our documentation!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 49689cf and pushed to 8.x. Thanks!

bneil’s picture

bneil commented
Version: 8.x-dev » 7.x-dev
Assigned: Unassigned » bneil
Status: Fixed » Needs work
bneil’s picture

bneil commented
Assigned: bneil » Unassigned
Status: Needs work » Needs review
StatusFileSize
new drupal-move_hook_requirements_install-2106783-6.patch3.26 KB

Here's a patch against 7.x. I don't believe $t = get_t() is necessary since this is a runtime phase requirement.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented

Patch looks good to me. Able to apply without any issues. +1 for changing it to RTBC.

sivaji_ganesh_jojodae’s picture

sivaji_ganesh_jojodae commented
Issue summary: View changes
Issue tags: +Novice
parthipanramesh’s picture

parthipanramesh commented

The latest patch works fine for me. Thanks! :)

parthipanramesh’s picture

parthipanramesh commented
Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

David_Rothstein commented
Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs backport to D7

Since the Node module is required in Drupal 7 (and even if it isn't, node_requirements() has no install-time checks anyway) there is no practical benefit to moving it. And since it's (sort of theoretically) a public API function that someone could be calling, I think it's best to just leave this where it is in Drupal 7.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.