The function media_query_media_browser_alter() in media.media.inc is incorrect. It states:

/**
 * Implements hook_query_media_browser_alter().
 */

but it actually implements hook_query_TAG_alter provided by system.module.

query_media_browser_alter is also listed as a media hook in media_hook_info. I believe this is incorrect and should be removed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Devin Carlson’s picture

Status: Active » Needs review
FileSize
785 bytes

A patch to correct the documentation and remove the hook from media_hook_info.

Since the function doesn't actually implement a media hook, I think it should be moved out of media.media.inc (probably into media.module), but I'll leave that for a separate issue.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks.
not committing it yet, since i dont want to break a patch by accident that people in atlanta might review

ParisLiakos’s picture

Status: Reviewed & tested by the community » Fixed
ParisLiakos’s picture

Status: Fixed » Needs work

i reverted this..removing the hook from hook_hook_info results to media_query_media_browser_alter() never being called..we need to move this function to the .module file

I would say we need tests for this..but i guess we need testbot first

Devin Carlson’s picture

A year and a half later with tests!

The tests exposed a few other issues, so including the fixes in this patch.

Status: Needs review » Needs work
Devin Carlson’s picture

Status: Needs work » Needs review
FileSize
3.16 KB

A better display of the test failures.

Status: Needs review » Needs work
Dave Reid’s picture

It's perfectly save to leave in the media_hook_info() part. That just needlessly breaks implementations in custom code if removed.

Devin Carlson’s picture

Status: Needs work » Fixed

Fair enough. :)

Committed only the doxygen change from the original patch.

Status: Fixed » Closed (fixed)

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