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.

CommentFileSizeAuthor
#10 1238118-media-browser-set-params.patch1.42 KBdave reid
#8 media_browser_params_alter-1238118-8.patch473 bytesidflood
#3 media_browser_params_alter-1238118-2.patch499 bytespp
#1 media_browser_params_alter-1238118-1.patch498 bytesaaron
media_browser_params_alter.patch499 bytespp

Comments

aaron’s picture

aaron commented
StatusFileSize
new media_browser_params_alter-1238118-1.patch498 bytes

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

aaron’s picture

aaron commented
Status: Needs review » Reviewed & tested by the community

rtbc, assuming it passes the test...

pp’s picture

pp commented
StatusFileSize
new media_browser_params_alter-1238118-2.patch499 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)

dave reid’s picture

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

!isset() is always preferred over is_null()

pp’s picture

pp commented

Why? Is it a Drupal wodo?

pp’s picture

pp commented

Isset is faster.

aaron’s picture

aaron commented

yes, !isset() is faster, and doesn't throw an error if passed a variable that hasn't yet been set. the only place where is_null() might be preferred is if passing in a function call, which is bad practice anyway.

idflood’s picture

idflood commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new media_browser_params_alter-1238118-8.patch473 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.

idflood’s picture

idflood commented
Status: Reviewed & tested by the community » Needs review
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 1238118-media-browser-set-params.patch1.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;
}
pp’s picture

pp commented
Status: Needs review » Reviewed & tested by the community

#10 It is the best solution.

dave reid’s picture

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

Committed #10 to both 7.x-2.x and 7.x-1.x.
http://drupalcode.org/project/media.git/commit/fda6f10
http://drupalcode.org/project/media.git/commit/1cda239

Status: Fixed » Closed (fixed)

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