This issue was recently brought to my attention in #2008388: hook_group_permission() / hook_permission collision with field_group module.

In the event of:

  • a module being named <module_a>
  • a module being named <module_a>_<module_b>
  • a hook being named hook_<hook_a>
  • a hook being named hook_<module_b>_<hook_a>

A naming collision can occur on a function with the name: <module_a>_<module_b>_<hook_a>.

Examples:

  • Collision on views_user_load
    • views + hook_user_load
    • views_user + hook_load
  • Collision on field_group_permission
    • field + hook_group_permission
    • field_group + hook_permission

Solution

hook_hook_implementation(), specifying if a module's implementation of a hook should deviate from the standard hook naming pattern.

About the patch

I don't know for sure whether we'll keep hooks as they are now in D8, as it will be much more OO. I therefore wrote a patch (attached in #1) against D7 already so you can get a general idea of what this would look like in D8.

Further reading

CommentFileSizeAuthor
#1 drupal-prevent-hook-name-collisions-2016003-1-D7-do-not-test.patch7.91 KBkristiaanvandeneynde
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented
FileSize
drupal-prevent-hook-name-collisions-2016003-1-D7-do-not-test.patch7.91 KB

Patch as promised.

Again: this is against D7 as an example.
I didn't want to write this against D8 while not knowing if hooks will continue to exist in it.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented
Status: Active » Needs review
lpalgarvio’s picture

lpalgarvio CreditAttribution: lpalgarvio commented
Issue summary: View changes
Issue tags: +Drupal core hooks, +backport
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented
Priority: Normal » Critical
Issue tags: +WSOD

Bumping to critical as this could lead to WSODs.

Since we never enforced module writers to not use underscores in their module names, it is now too late to fix the collision issue that way. The proposed patch is one way of fixing it, although there could be other ways as well.

There are numerous naming patterns already where this could crash Drupal:

  • <existing_module>_entity: crashes on hook_load and hook_entity_load
  • <existing_module>_user: crashes on hook_load and hook_user_load
  • <existing_module>_group: crashes on hook_permission and hook_group_permission
  • ...
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde commented
Issue summary: View changes

Clarified example in summary.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Closed (duplicate)

Duplicate of #548470: Use something other than a single underscore for hook namespacing.

donquixote’s picture

donquixote CreditAttribution: donquixote as a volunteer commented

@kristiaanvandeneynde I looked into your proposal.

I think anything that would introduce an alternative naming pattern would currently be problematic for backwards compatibility in Drupal 7 or 8.

The most important problem I can think of is modules that have custom versions of module_invoke_all(), and which do not know about the alternative naming syntax.

Instead, I would propose a way for modules to register explicitly which hooks they implement.
If a module 'aaa' does this, it would have the following effect:

  • A function aaa_bbb_ccc() that is NOT explicitly registered as 'aaa' + 'bbb_ccc' will not be part of module_implements('bbb_ccc'), but it might still be part of module_implements('ccc') for module 'aaa_bbb'.
  • A function aaa_bbb_ccc() that IS registered as 'aaa' + 'bbb_ccc' will NOT be part of module_implements('ccc') for module 'aaa_bbb'.

Maybe this should be a new issue since the proposal here is quite different.
Or maybe I should start with a contrib module.

donquixote’s picture

donquixote CreditAttribution: donquixote as a volunteer commented

Here is a module which provides an API for Drupal 7 for modules to specify a whitelist of hook implementations.
https://github.com/donquixote/drupal-clashnomore
https://www.drupal.org/sandbox/donquixote/2819055

Use cases:

1. A module implements 5 hooks, and wants that
- no other functions are interpreted as implementations of any hook by this module.
- the 5 hook implementations are never interpreted as implementations of any other hook, by another module.

2. A module knows about a problematic function like field_group_permission(), and wants to declare that the correct interpretation is field_group + permission, not field + group_permission.

See the clashnomore.api.php and also the module's own implementation of both hooks it provides, in clashnomore.module.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
CreditAttribution: kristiaanvandeneynde at Deeson commented

Opting in to hooks will still not prevent clashes, though. Just unintended ones.

Assuming field_group_permission, what if both Field wants to implement hook_group_permission and Field Group wants to use hook_permission? We'd still have two functions with the exact same name.

donquixote’s picture

donquixote CreditAttribution: donquixote as a volunteer commented

This is correct.
But I think this is the closest we can get without causing BC issues.

The kind of nameclash you mention is far less likely to happen.
If we were to design a new syntax, then I would want it to be bullet-proof against all name clashes. But as a solution within Drupal 7 or Drupal 8, I think this might be "good enough".