hook_media_browser_params_alter hook doesn't work. [#1238118]

I think the media_set_browser_params() function have two error:

1. If $params is an empty array the media_set_browser_params() function don't call hook_media_browser_params_alter() hook, because PHP cast empty array to FALSE like NULL. I suggest use "===" instead of "!".

2. hook_media_browser_params_alter hook call whith $stored_params, not $params. (why use & there?)

please see the patch.

The patch works 7.x-2.x-dev and 7.x-1.x also.

Comment	File	Size	Author
#10	1238118-media-browser-set-params.patch	1.42 KB	Dave Reid
#10
#8	media_browser_params_alter-1238118-8.patch	473 bytes	idflood
#8
#3	media_browser_params_alter-1238118-2.patch	499 bytes	pp
#3
#1	media_browser_params_alter-1238118-1.patch	498 bytes	aaron
#1
	media_browser_params_alter.patch	499 bytes	pp

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

aaron CreditAttribution: aaron commented 3 August 2011 at 22:00

File	Size
media_browser_params_alter-1238118-1.patch	498 bytes

this all looks right. except i think you should use !isset() rather than == NULL... the attached patch does that.

Comment #2

aaron CreditAttribution: aaron commented 3 August 2011 at 22:02

Status:

Needs review

» Reviewed & tested by the community

rtbc, assuming it passes the test...

Comment #3

pp CreditAttribution: pp commented 4 August 2011 at 04:20

File	Size
media_browser_params_alter-1238118-2.patch	499 bytes

ok, but i think we should use is_null() rather than !isset(). The $params variable always exists. We shouldn't test the $params variable exist or not (isset), We sould test the $params variable is null or not. (is_null, isset)

Comment #4

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 4 August 2011 at 04:21

!isset() is always preferred over is_null()

Needs review

» Reviewed & tested by the community

File	Size
media_browser_params_alter-1238118-8.patch	473 bytes

I have no doubt in the first point exposed in #1. But the second about $stored_params seems a bit wrong to me. At least, the solution provided in #1245430: media_set_browser_params should use reference of $params looks more coherent.

Here is a patch with a mix of both solution. I don't know how to test this manually though.

Comment #9

idflood CreditAttribution: idflood commented 11 August 2011 at 12:30

Status:

Reviewed & tested by the community

» Needs review

Comment #10

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 11 August 2011 at 14:04

Status:

Reviewed & tested by the community

» Needs review

File	Size
1238118-media-browser-set-params.patch	1.42 KB

I think this is actually the more proper solution:

  $params = media_set_browser_params($params);

...

function media_set_browser_params(array $params = NULL) {
  $stored_params = &drupal_static(__FUNCTION__, array());

  if (isset($params)) {
    $stored_params = $params;
    // Allow modules to alter the parameters.
    drupal_alter('media_browser_params', $stored_params);
  }

  return $stored_params;
}