Problem

hook_ENTITY_TYPE_ACTION() has no prefix, which results in pseudo-random unprefixed hook names, and consequently this causes conficts. I have a local dev site with payment.module and commerce_payment.module enabled. Payment has a "payment_method" entity type and Commerce Payment has the commerce_payment_method_load() API function. When loading a payment_method entity (the one from Payment.module), Entity API fires hook_payment_method_load() and commerce_payment_method_load() is called. Apart from having completely different uses, both functions accept different arguments, which causes PHP errors to be thrown by commerce_payment_method_load().

Solution

Convert hook_ENTITY_TYPE_ACTION() to hook_entity_ENTITY_TYPE_ACTION(). This is the official behavior for hooks (to be prefixd with the name of the module defining them) and it will prevent the problem.

CommentFileSizeAuthor
#10 entity_1549424_03.patch3.59 KBxano
#8 entity_1549424_02.patch3.59 KBxano
#1 entity_optionally_disable_entity-type_specific_hooks_01.patch2.16 KBxano

Comments

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs work » Needs review
StatusFileSize
new entity_optionally_disable_entity-type_specific_hooks_01.patch2.16 KB

This patch adds an "entity-type_specific_hooks" property to hook_entity_info() that prevents entity-type specific hooks (note the property's spelling) from being fired if this property is set to FALSE in hook_entity_info(). This makes sure existing Entity API implementations will continue to function.

fago’s picture

fago
Primary language German
Location Vienna
commented
Assigned: Unassigned » xano
Status: Active » Needs work

Convert hook_ENTITY_TYPE_ACTION() to hook_entity_ENTITY_TYPE_ACTION(). This is the official behavior for hooks (to be prefixd with the name of the module defining them) and it will prevent the problem.

Entity API provides the hooks on top of the entity-providing module, which has to own the entity-type namespace. That's fine in the case of payment_method too.

The issue you ran into is actually a generic issue with hook namespaces that might occur with modules using prefix separated by _. We cannot fix this issue in the entity api of course, still we can add an option as proposed by the patch. But let's make the option a bit nice:

What about making the hook_base_name configurable, while default to the entity type? In case the hook base name is FALSE, we do not create those hooks. Maybe just call it 'hook base' ? Existing hook-configuration keys which are specific to a certain hook should take precedence though.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

With "hook_base_name", do you mean an existing property or is that the property I proposed with a more general name? If so, its name suggest it does more than configuring entity-specific action hooks, which may cause some confusion.

Entity API provides the hooks on top of the entity-providing module, which has to own the entity-type namespace. That's fine in the case of payment_method too.

I understand, but IMHO it's a bit tricky to use another module's namespaces like Entity does. It's not the flawed feature I said it was (I apologize for that), but it certainly has its challenges.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs review » Needs work

Bump.

silkogelman’s picture

silkogelman commented

issue confirmed:

Warning: Illegal offset type in isset or empty in commerce_payment_method_load() (line 576 of /var/www/XXX/docroot/profiles/commerce_kickstart/modules/contrib/commerce/modules/payment/commerce_payment.module).
Warning: Illegal offset type in isset or empty in commerce_payment_method_load() (line 576 of /var/www/XXX/docroot/profiles/commerce_kickstart/modules/contrib/commerce/modules/payment/commerce_payment.module).
Warning: Illegal offset type in isset or empty in commerce_payment_method_load() (line 576 of /var/www/XXX/docroot/profiles/commerce_kickstart/modules/contrib/commerce/modules/payment/commerce_payment.module).
Warning: Illegal offset type in isset or empty in commerce_payment_method_load() (line 576 of /var/www/XXX/docroot/profiles/commerce_kickstart/modules/contrib/commerce/modules/payment/commerce_payment.module).
Warning: Illegal offset type in isset or empty in commerce_payment_method_load() (line 576 of /var/www/XXX/docroot/profiles/commerce_kickstart/modules/contrib/commerce/modules/payment/commerce_payment.module).
Warning: Illegal offset type in isset or empty in commerce_payment_method_load() (line 576 of /var/www/XXX/docroot/profiles/commerce_kickstart/modules/contrib/commerce/modules/payment/commerce_payment.module).

When saving a Payment method for the Payment module.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Bump. Fago, could you please advise on which solution will work for you? I'll work on a patch, but before I can start working on that I need to know which approach you believe is most suitable. Thanks! :)

fago’s picture

fago
Primary language German
Location Vienna
commented

I think we should do it as following:

What about making the hook_base_name configurable, while default to the entity type? In case the hook base name is FALSE, we do not create those hooks. Maybe just call it 'hook base' ? Existing hook-configuration keys which are specific to a certain hook should take precedence though.

Let's invent this 'hook base' key and follow it.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs work » Needs review
StatusFileSize
new entity_1549424_02.patch3.59 KB

Status: Needs review » Needs work

The last submitted patch, entity_1549424_02.patch, failed testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs work » Needs review
StatusFileSize
new entity_1549424_03.patch3.59 KB
silkogelman’s picture

silkogelman commented

Quickly tested with 7.x-1.x-dev (2012-Sep-07) in a Commerce Kickstart v2 setting.
The patch applies fine, but it keeps hitting me with the warning (maybe I am doing something wrong)

Using a 'clean' Commerce Kickstart v2 install with Payment module and Payment Commerce module.

Will retest under different circumstances at a later point in time (with a more up2date Commerce Kickstart v2 or clean Commerce install)

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

That's correct, because Payment doesn't implement this new option yet. I'll write a patch for that later today.. The original problem has been fixed in #1819626: Entity hook collission using hook_module_implements_alter(), which means this issue can no longer be tested using Payment.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Assigned: xano » Unassigned
chris matthews’s picture

chris matthews commented
Issue summary: View changes
Status: Needs review » Needs work