Download & Extend
Issues

Remove hook_hook_info_alter()

Posted by sun on January 18, 2013 at 6:31pm
Project:Drupal core
Version:8.x-dev
Component:base system
Category:task
Priority:normal
Assigned:Dave Reid
Status:closed (fixed)
Issue tags:API clean-up

Issue Summary

hook_hook_info_alter(), really?

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

Change records for this issue

Login or register to post comments

Comments

#1

Posted by Dave Reid on January 21, 2013 at 7:33pm

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...

#2

Posted by sun on January 21, 2013 at 7:37pm

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).

#3

Posted by sun on January 22, 2013 at 12:54pm
Title:Remove hook_hook_info_alter(). It's waste.» Remove hook_hook_info_alter()
Status:active» needs review

Here's a patch.

AttachmentSizeStatusTest resultOperations
drupal8.hook-hook-info-alter.3.patch1.51 KBIdlePASSED: [[SimpleTest]]: [MySQL] 50,666 pass(es).View details

#4

Posted by sun on January 24, 2013 at 10:50am

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.

#5

Posted by Crell on January 24, 2013 at 9:58pm

+1 to #4.

#6

Posted by catch on January 25, 2013 at 12:36pm

Works for me as well.

#7

Posted by sun on January 25, 2013 at 4:56pm
Status:needs review» reviewed & tested by the community

#8

Posted by chx on January 26, 2013 at 6:24am
Assigned to:sun» Dave Reid
Status:reviewed & tested by the community» needs review

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

#9

Posted by Dave Reid on January 26, 2013 at 6:28am
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.

#10

Posted by Dave Reid on January 26, 2013 at 6:30am
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.

#11

Posted by Dave Reid on January 26, 2013 at 7:26am
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.

#12

Posted by catch on January 28, 2013 at 10:51pm
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 notification

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

#13

Posted by Crell on January 29, 2013 at 12:55am
Title:Change notice: Remove hook_hook_info_alter()» Remove hook_hook_info_alter()
Priority:critical» normal
Status:active» fixed
Issue tags:-Needs change notification

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

#14

Posted by System Message on February 12, 2013 at 1:00am
Status:fixed» closed (fixed)

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

nobody click here