hook_hook_info_alter(), really?

Is anyone able to make up a braindead use-case for that? :)

CommentFileSizeAuthor
#3 drupal8.hook-hook-info-alter.3.patch1.51 KBsun

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Yes, I've used it to provide support for existing core hooks since two of the same keys causes an array merge of invalid data.

EDIT: That way if core adds support for those hooks, my custom code doesn't cause hooks to break.

http://drupalcontrib.org/api/drupal/contributions%21file_entity%21file_e...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I guess you're referring to the behavior of array_merge_recursive()?

I think that should be resolved by now, since we're using NestedArray::mergeDeep() for the information gathered by info hooks now?

Can you confirm? If not, that might translate this issue to won't fix (I hope not).

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Remove hook_hook_info_alter(). It's waste. » Remove hook_hook_info_alter()
Status: Active » Needs review
StatusFileSize
new drupal8.hook-hook-info-alter.3.patch1.51 KB

Here's a patch.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

I'm fairly sure that the usage of NestedArray::mergeDeep() in HEAD eliminates the need for using hook_hook_info_alter() for cases like #1.

So how about this:

  1. We remove it from core now.
  2. If it turns out that contrib still needs it, we can consider to re-introduce it, or fix whatever else needs to be fixed.
Crell’s picture

Crell commented

+1 to #4.

catch’s picture

catch
he/him
Primary language English
commented

Works for me as well.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
chx’s picture

chx commented
Assigned: sun » dave reid
Status: Reviewed & tested by the community » Needs review

Given that Dave had a problem -- does this work for you?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Needs work

It appears that module_hook_info() for Drupal 8 still uses array_merge_recursive. That would need to be fixed along with this patch before it could be committed. Would be good to actually have a test that confirms two duplicate entries in hook_hook_info() don't cause things to explode since it's not as typically of a hook invocation as usual.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review

Wait a sec, appears I'm a victim of api.drupal.org being very out of date. Attempting to confirm locally.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Reviewed & tested by the community

Ok had a chance to completely reset a very screwed up and broken D8 sandbox and pull the current code. I confirmed that duplicate hook_info() results are in fact merged properly.

I did run into a WTF with ModuleHandler: #1899514: ModuleHandlerInterface::getHookInfo should not be protected.

catch’s picture

catch
he/him
Primary language English
commented
Title: Remove hook_hook_info_alter() » Change notice: Remove hook_hook_info_alter()
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed/pushed to 8.x. We should add a change notice.

Crell’s picture

Crell commented
Title: Change notice: Remove hook_hook_info_alter() » Remove hook_hook_info_alter()
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Change notice added: http://drupal.org/node/1901550

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