if more than one hook_openid implementation returns a given parameter, the resulting value is an array and request is invalid

+1 for this being a bug, and your proposed solution to it as well.

The OpenID 2.0 Final specification defines messages as single value key=value pairs. My Emphasis in bold:

4.1. Protocol Messages

The OpenID Authentication protocol messages are mappings of plain-text keys to plain-text values. The keys and values permit the full Unicode character set (UCS). When the keys and values need to be converted to/from bytes, they MUST be encoded using UTF-8 [RFC3629].

Messages MUST NOT contain multiple parameters with the same name.

see: http://openid.net/specs/openid-authentication-2_0.html#anchor3

Yet Drupal 6 openid.module is treating a value that may be an Array() as a scalar, in both openid_redirect() and openid_redirect_http().

openid_redirect_http(), openid.inc line 30:

  foreach ($message as $key => $val) {
    $query[] = $key .'='. urlencode($val);    // BUG: If two modules answered hook_openid(), this $val may be an array()!
  }

and function openid_redirect_form, openid.inc line 55: (this provides the Javascript form for OpenID 2.0 redirects)

  foreach ($message as $key => $value) {
    $form[$key] = array(
      '#type' => 'hidden',
      '#name' => $key,
      '#value' => $value,   // BUG:  $value may contain an array()
    );

So as can be clearly seen from inspecting the code, it's easy to un-intentionally violate the protocol when two modules both set the same request parameter from hook_openid().

I agree with Barrett that the openid module should take steps to prevent this. The correct behaviour should be that the module with the highest weight that answers hook_openid() last will "win". This is accomplished with the patch from #1.

Finally, this behaviour is present in HEAD as well. Line 776 of 8.x core/modules/openid/openid.module also contains:

$request = array_merge($request, module_invoke_all('openid', 'request', $request));

I'm therefore marking this as 8.x. We should fix it there, then backport the bugfix to D7 and D6.

I will come up with a simpletest we can use to replicate the problem and verify the fix. It might as well test the entire request object and make sure it conforms to the specification of unique key names with scalar (single) values.

Meanwhile, can someone offer some feedback on Barrett's solution of flattening the array? Perhaps there is a better way or a more Drupal way?

The bug does not exist in D8. It was fixed in #765984: Convert hook_openid('request', ...) to hook_openid_request_alter(). But that issue changes the API, so we cannot backport it to D7 as is.

I think the idea about just using the last value is fine. AFAICT the values in $request are either strings or arrays of strings (assuming that modules are well-behaved and only return arrays of strings), so there is no need to for _flatten_request_element() to call itself recursively(). Let's just keep everything inside openid_authentication_request().

#4: flatten-request-1379056-4.patch queued for re-testing.

#1: flatten-request-1379056.patch queued for re-testing.

Couldnt apply both patches. The output being 'patch corrupt' and apparantly it does have some errors which are critical.
For example in http://drupal.org/node/1379056#pift-results-1379056 at line:40 comments are not closed and on line: 39 the '}' should follow '+' and it didnt.

#4: flatten-request-1379056-4.patch queued for re-testing.

 root@bob:/var/www/drupal/drupal# git apply flatten-request-1379056-4.patch 
flatten-request-1379056-4.patch:30: trailing whitespace.
 
error: patch failed: modules/openid/openid.module:787
error: modules/openid/openid.module: patch does not apply

This is the error I am having while applying the patch(the most recent one as you said).

+  // Make sure the request only has simple key=>value pairs, not arrays

I think this comment needs a bit of elaboration to stress that arrays appears when two modules specify the same parameter, i.e. not only if some broken module returns an array. I suggest you look into the comment in rdf_get_namespaces() for inspiration. Don't forget the trailing period.

Apart from this nit, I think this patch is ready to fly.

Hm, as far as I can tell, this bug was actually worse - if you followed the hook_openid() documentation example and returned the whole $request object (rather than just the parameters you're adding) this would occur any time two modules both implemented the hook, rather than only if they happened to alter the same parameter.

I'm a little concerned there's no test coverage for this (or explicit hook documentation about the behavior), but then again there is no existing test coverage for the hook and it might be hard to test...

So I committed this in the interest of getting the bug fixed, and because it seemed straightforward and worked as expected on manual testing. Thanks! http://drupalcode.org/project/drupal.git/commit/0bcf41a

Maybe there can be a followup for tests and/or documentation?